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

Add webhook provider (Proof of Concept) #409

Closed
wants to merge 2 commits into from

Conversation

tmayr
Copy link
Contributor

@tmayr tmayr commented Jul 8, 2020

This is 101% a proof of concept that needs to be cleaned up, it has my prettier configuration applied so even the review is a bit messy, just focus on the big code blocks which were added more than anything.

There's a lot of repetition as I mentioned, because the email provider is 90% similar, so taking on the same flow, I mostly copied/pasted and made it a bit more generic with taking attribute and value instead of hardcoding email and assuming it.

It also changes how the identifier is used, before it was just the email value, now its attribute:value

For now, phoneNumber is hardcoded in some parts, but thinking this would come from the provider configuration (say you specify { validationAttribute } or something like that, so you could specify whatever you want.

Will leave some comments on the blocks that are actually behavior change

@iaincollins Take a look whenever you can!, will wait for v3 before polishing this based on whatever comes up.

Refs #406

@vercel
Copy link

vercel bot commented Jul 8, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/iaincollins/next-auth-docs/8abmtzic8
✅ Preview: https://next-auth-docs-git-fork-tmayr-add-webhook-provider.iaincollins.vercel.app

@vercel vercel bot temporarily deployed to Preview July 8, 2020 14:46 Inactive
name: "Webhook",
maxAge: 24 * 60 * 60, // How long link are valid for (default 24h)
...options,
sendVerificationRequest: ({
Copy link
Contributor Author

@tmayr tmayr Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is "inherited" from the email provider, nothing says it couldn't change.

You would do whatever you want here, like, sending the email, sending an sms, hitting a webhook

}
} else if (providerAccount.type === "webhook") {
// If signing in with an email, check if an account with the same email address exists already
const userByEmail = profile.phoneNumber
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phoneNumber is hardcoded here, need to find out a way to pass in the attribute to this method, and obviously names are off because of copying/pasting email behavior

} else if (providerAccount.type === "webhook") {
// If signing in with an email, check if an account with the same email address exists already
const userByEmail = profile.phoneNumber
? await getUserByAttribute("phoneNumber", profile.phoneNumber)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change from the email, getUserByAttribute let's you specify an attribute name and a value instead of assuming email

}
}

// // Update emailVerified property on the user object
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to figure out how to deal with this, maybe have a verificationProperty on the provider so we know which field to set to true when verification.

@@ -0,0 +1,37 @@
import { randomBytes } from "crypto";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact same as src/server/lib/signin/email.js but recieves an extra value ((attribute, value, provider, options) instead of (email, provider, options))


// @TODO Create invite (send secret so can be hashed)
await createVerificationRequest(
attribute + ":" + value,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We change the identifier here to attribute:value to keep track of what we're validating in the db

);
res.end();
return done();
} else if (provider.type === "webhook") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would read from the body to get the correct info and pass it on, implementation is missing mainly because of this being a PoC, not sure how similar its gonna be to email.

@tmayr
Copy link
Contributor Author

tmayr commented Jul 8, 2020

Left tons of comments on the relevant parts, excuse the prettier mess!

}
}

async function getUserByEmail (email) {
debugMessage('GET_USER_BY_EMAIL', email)
async function getUserByAttribute(attribute, value) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically the same as getUserByEmail but with 2 params

@iaincollins
Copy link
Member

Thank you for all the comments on this! <3

@iaincollins
Copy link
Member

Still working on the backlog, but will circle back to this!

@stale
Copy link

stale bot commented Dec 5, 2020

Hi there! It looks like this issue hasn't had any activity for a while. It will be closed if no further activity occurs. If you think your issue is still relevant, feel free to comment on it to keep ot open. Thanks!

@stale stale bot added the stale Did not receive any activity for 60 days label Dec 5, 2020
@balazsorban44 balazsorban44 changed the base branch from main to canary December 6, 2020 20:50
@stale stale bot removed the stale Did not receive any activity for 60 days label Dec 6, 2020
@balazsorban44
Copy link
Member

Hi @tmayr, would you be interested in getting this up-to-date?

@tmayr
Copy link
Contributor Author

tmayr commented Feb 2, 2021

@balazsorban44 Sure, any insights on what has changed (anything worthy mentioning)/or if theres any extra modifications the team would like me to apply/explore conceptually?

@balazsorban44
Copy link
Member

Hi there! So after a recent chat with another maintainer of this repo, we discussed how we could expand on the email provider to support similar ways of authentication, like SMS as well. I am going to close this PR for now, and soon create an RFC for this new provider, which we probably will call something like token provider or something. Your PR has had an influence on me, so thank you!

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.

3 participants