Skip to content
This repository has been archived by the owner on Aug 6, 2023. It is now read-only.

Email link provider #4

Open
waspeer opened this issue Aug 22, 2022 · 4 comments
Open

Email link provider #4

waspeer opened this issue Aug 22, 2022 · 4 comments

Comments

@waspeer
Copy link

waspeer commented Aug 22, 2022

Hi!

Thanks for making this package! This makes working with Astro even better :)

For my use case I was able to get an email magic link flow working with the CredentialsProvider, which speaks to the amazing modularity of this package. But I think it would be even better if an EmailProvider was included in this project by default. If you'd be open to it, I would like to contribute create a PR for this. This is my proposal:

1) Send verification email

The flow starts by sending a verification token to the users email. This wil most likely be triggered from the client:

signIn({
  provider: 'email',
  sendToken: {
    email: 'EMAIL',
  },
});

2) Credentials hook

It would be nice if the developer could hook in to this step to allow/prevent sending the token. As I understand the signIn hook is triggered after authentication? So, it might make sense to introduce a new hook that is triggered before authenticating the user when providing credentials:

export const all = AstroAuth({
  authProviders: [
    // ...
  ],
  hooks: {
    credentials: async (credentials) => {
      if (credentials.provider === 'email') {
        const email = credentials.sendToken.email;
        return emailAllowList.includes(email);
      }

      if (credentials.provider === 'crendentials') {
        const email = credentials.login.email;
        return emailAllowList.includes(email);
      }

      // ...
    },
  },
});

This hook can be used to allow/deny credentials in the EmailProvider as well as the CredentialsProvider.

Another option is to reuse the signIn hook. This is what next-auth does. I personally think this is confusing, especially since the user object included in the arguments is just a placeholder in this case.

3) Generate Token

When the credentials are allowed the next step is to generate a token. My implementation generates a random string and persists that in a database, similar to how next-auth does it.

Pseudocode
import { nanoid } from 'nanoid';

async function generateToken(email: string) {
  const verificationToken = {
    email,
    token: nanoid(),
    createdAt: new Date(),
  };

  await persistVerificationToken(verificationToken);

  return verificationToken.token;
}

async function verifyToken(token: string, email: string) {
  const verificationToken = await getVerificationTokenFromDb(token);

  if (
    !verificationToken ||
    verificationToken.createdAt.getTime() + MAX_AGE < Date.now() ||
    verificationToken.email !== email
  ) {
    return false;
  }

  return true;
}

Another option is to encrypt a string including the email and createdAt using the ASTROAUTH_SECRET environment variable and use this as a token. This is similar to how remix-auth-email-link does it.

Pseudocode
import * as AES from 'crypto-js/aes';
import utf8Encoder from 'crypto-js/enc-utf8';

async function generateToken(email: string) {
  const verificationToken = {
    email,
    createdAt: Date.now(),
  };

  const encryptedToken = AES.encrypt(JSON.stringify(verificationToken), SECRET);

  return encryptedToken;
}

async function verifyToken(token: string, email: string) {
  let verificationToken: Record<string, any>;

  try {
    const json = AES.decrypt(token, SECRET).toString(utf8Encoder);
    verificationToken = JSON.parse(json);
  } catch {
    return false;
  }

  if (
    !verificationToken ||
    verificationToken.createdAt + MAX_AGE < Date.now() ||
    verificationToken.email !== email
  ) {
    return false;
  }

  return true;
}

I personally think for this library it makes more sense to go with the second option, since the whole adapter/persistence layer is not present atm. This flow could optionally be enriched by storing the encrypted token in a cookie and requiring the token in the cookie and the token send to the verification callback to be equal (this option is also provided in remix-auth-email-link).

4) Sending email

next-auth has nodemailer as an optional peer dependency and by default handles sending the email for you. Of course SMTP credentials need to be provided.

remix-auth-email-link requires the developer to define a sendEmail function to send the email.

I would personally like to go for the latter, even though it requires a bit more setup for the developer.

5) Verifying the token

I think it would make sense to add another auth endpoint called verify-email-token that requires the token as a search parameter and possibly an email parameter. I already added pseudocode for verifying the token in the generate token section.


I would love to hear your thoughts before I start working on a PR.

@waspeer
Copy link
Author

waspeer commented Aug 29, 2022

Just following up on this. I'm also happy to just start on a pr if that works better?

@ran-dall
Copy link
Contributor

@waspeer Thank you for your interest in contributing to the Astro Community. It's great seeing more people interested in taking the initiative and contributing. Sorry for the delay in getting back to you; @osadavc has been preoccupied, and so has the Astro team post-v1; I was just able to set some time aside to reply. As for your suggestion, it's most defiantly a good one. My main concern is how this affects the attack surface of the package. This concern is further amplified when you consider the current security state of the package. Beyond that, I don't see any problem integrating your idea into the integration.

I suggest we wait for a week (or two?), if possible. I'm already working on several changes required to improve our current security posture that should be pushed by then. I think then we should revisit this conversation and see how we can work together to integrate your suggestion.

@waspeer
Copy link
Author

waspeer commented Sep 12, 2022

Hey @ran-dall,

All good! Just let me know when/how you want to proceed.

As I mentioned in the issue, I have a working example already (built on top of the CredentialsProvider). I expect most of that code can be used for this integration, if you don't see any security vulnerabilities in the approach I outlined above.

@waspeer
Copy link
Author

waspeer commented Oct 4, 2022

Friendly little follow-up ✨

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants