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

Closed
wants to merge 3 commits into from
Closed

[ Feat ]: Phone Support #65

wants to merge 3 commits into from

Conversation

shawnmclean
Copy link

@shawnmclean shawnmclean commented May 27, 2024

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

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

dev-xo commented May 27, 2024

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 dev-xo requested a review from mw10013 May 27, 2024 20:24
@dev-xo
Copy link
Owner

dev-xo commented May 27, 2024

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:

  • A good way to validate phones (also exposing that validation to the user in the same way we are exposing the email validation).
  • Adding some tests to ensure everything behaves as expected.

validateEmail?: ValidateEmail

this.validateEmail = options.validateEmail ?? this._validateEmailDefault

@dev-xo dev-xo added the enhancement New feature or request label May 27, 2024
@dev-xo dev-xo changed the title draft: Phone Support [ Feat ]: Phone Support May 27, 2024
@mw10013
Copy link
Collaborator

mw10013 commented May 28, 2024

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

shawnmclean commented May 28, 2024

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

dev-xo commented May 28, 2024

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:

  • Documenting possible services they could use to bypass phone authentication (the service sending the SMS)
  • Documenting having only one single field, either email or phone
  • Some other possible notes that could arise or could be important in order to set this up properly.

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

  • Validating phones (also exposing that validation to the user).
  • Adding tests to ensure everything behaves as expected.
  • Documenting its possible usage and keeping it simple and easy to implement.

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:

  • Can the newly added context handle this authentication method somehow? Haven’t looked into the context update that much, so probably this has been contemplated or is simply not possible to achieve through it.

@mw10013
Copy link
Collaborator

mw10013 commented May 29, 2024

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

chiptus commented Jun 4, 2024

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

mw10013 commented Jun 6, 2024

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

chiptus commented Jun 7, 2024

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

mw10013 commented Jun 8, 2024

@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

/**

  • Call this method with the Strategy, the optional name allows you to setup
  • the same strategy multiple times with different names.
  • It returns the Authenticator instance for concatenation.
  • @example
  • authenticator
  • .use(new SomeStrategy({}, (user) => Promise.resolve(user)))
  • .use(new SomeStrategy({}, (user) => Promise.resolve(user)), "another");
    */
    use(strategy: Strategy<User, never>, name?: string): Authenticator {
    this.strategies.set(name ?? strategy.name, strategy);
    return this;
    }

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.

@dev-xo dev-xo closed this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants