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 example with Magic authentication #11810

Merged
merged 15 commits into from Apr 22, 2020
Merged

Add example with Magic authentication #11810

merged 15 commits into from Apr 22, 2020

Conversation

seanli
Copy link
Contributor

@seanli seanli commented Apr 10, 2020

Hi I'm Sean, co-founder of Magic! This pull request adds an official example of using Magic to implement cookie-based, passwordless authentication with email-based magic links.

This work is based on the existing with-passport example (following existing layout and styles), removed dependencies on passport and express, and influenced by @emwsc's Pull Request: #11678

Demo: https://magic-next.lisean.now.sh

@emwsc
Copy link

emwsc commented Apr 11, 2020

Should i close my PR? I think having official example is better.

@seanli
Copy link
Contributor Author

seanli commented Apr 11, 2020

That sounds good @emwsc, and thanks so for the work in your PR! The environment variable stuff was really useful. Thought I'd add our passport-magic in the example so that developers can learn how to do session management themselves with Magic if they like.

@janus-reith
Copy link

@seanli In the login api route you use express, probably for passport compatibility.
I'm not sure how much of an overhead that gives, so maybe that actually is the way to go if you rely on express specific features, however, I can confirm that Passportjs will also work in API routes without express.

There is one caveat, Micro (That's what the API Routes use) lacks a redirect function on res and many Passport strategies make use of that, so you may need to provide a custom
redirect-function on res if your strategy makes use of redirects, I did'nt check that.
That function basically just would be this:
(Credit: https://todayilearned.io/til/nextjs-with-passport-oauth-cookie-sessions)

res.redirect = (location) => {
	res.statusCode = 302;
	res.setHeader("Location", location);
	res.end();
}

@seanli
Copy link
Contributor Author

seanli commented Apr 11, 2020

@janus-reith I tried to be as consistent as possible with the with-passport example (https://github.com/zeit/next.js/tree/canary/examples/with-passport) and didn't really change much. In mine and the with-passport example express.js is used and passport redirect functionality is not used and a useUser hook is used instead (https://github.com/zeit/next.js/blob/canary/examples/with-passport/lib/hooks.js).

Also Magic doesn't use oauth and doesn't force redirects, it adopts a decentralized identity (https://w3c-ccg.github.io/did-primer/) architecture where users sign claims with their own private key instead of a trusted 3rd party centralized identity provider, which give developers more flexibility to decide whether to redirect or not. (More about our architecture: https://docs.magic.link/security)

Let me know what you think and whether I should stay consistent with the with-passport example or change it to something else.

@janus-reith
Copy link

@seanli You're right, TBH I didn't check that example before and just noticed yours, I'd suggest the same for that other one aswell then.

But that's just my opinion, it's probably best to wait for the opinion of the core nextjs dev team what would be better before you put effort into changing it.

@timneutkens
Copy link
Member

There is one caveat, Micro (That's what the API Routes use)

This is incorrect, API routes do not use Micro.

examples/magic/README.md Outdated Show resolved Hide resolved
Removed Download manually section from README

Co-Authored-By: Joe Haddad <timer150@gmail.com>
@seanli seanli requested a review from Timer April 13, 2020 17:21
@lfades
Copy link
Member

lfades commented Apr 13, 2020

This is an alternative to Express: #11747 - If there's an even better alternative it's also welcome.

I'll review the PR soon 😌


## How to use

### Using `create-next-app`
Copy link
Member

Choose a reason for hiding this comment

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

@Timer Should we remove this title too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lfades @Timer following up on this!~

@seanli seanli requested a review from lfades April 16, 2020 13:21
@janus-reith
Copy link

There is one caveat, Micro (That's what the API Routes use)

This is incorrect, API routes do not use Micro.

Oh, I'm sorry for spreading misinformation then, I was under the assumption.
Not sure though where I got this from - Would it be correct to state that API routes have similar functionality to the ones of micro or is that not the case either?

@timneutkens
Copy link
Member

@janus-reith in general saying it's Micro-like causes confusion as Micro has a bunch of functionality that we intentionally left out of API routes, eg async bootup (not compatible with serverless functions), the JSON parsing function is separate and not automatic (req.body is not populated by micro, instead you have to await json(req)), cookies are not parsed, querystring is not parsed either in Micro. Most notably API routes don't rely on return to resolve the request (eg in Micro you can return { my: 'json' } and it automatically returns JSON, however this was counter-intuitive as sometimes you have work that has to happen asynchronously, so you'd use return in some places and send() (a helper exposed by Micro) in others. Basically we took our experience building Micro and using it for all of our API endpoints and solved most of the points that Micro could have been more ergonomic.

@janus-reith
Copy link

@timneutkens oh, agreed, so it really is not helpful to compare to micro at all. Thank you for the details!

@seanli seanli marked this pull request as draft April 19, 2020 18:32
@seanli
Copy link
Contributor Author

seanli commented Apr 19, 2020

Converted status to draft, thinking of removing the express dependencies to make the example more native and lightweight

@emwsc
Copy link

emwsc commented Apr 19, 2020

Like mine initial PR? :)

@seanli seanli changed the title Add example with Magic and Passport.js Add example with Magic authentication Apr 21, 2020
@seanli seanli marked this pull request as ready for review April 21, 2020 02:26
@seanli
Copy link
Contributor Author

seanli commented Apr 21, 2020

@Timer @lfades I took another stab at making this example cleaner and more lightweight, and also removed dependency on express and passport! PTAL 🙏

@lfades
Copy link
Member

lfades commented Apr 22, 2020

@seanli 💯. I'll be reviewing the PR soon, thank you

@lfades
Copy link
Member

lfades commented Apr 22, 2020

Hi @seanli The example is great!, and also the UI of magic.link is awesome, good work 💯

I have 3 commits to do, unfortunately I keep getting access denied when trying to push to your branch, can you check that updates from collaborators is enabled 🙏

@seanli
Copy link
Contributor Author

seanli commented Apr 22, 2020

Super awesome to hear that - thanks so much @lfades! 🔥 I just sent over a collaborator invite to you https://github.com/fortmatic/next.js/invitations - let me know if this works! 🙏

@lfades
Copy link
Member

lfades commented Apr 22, 2020

@seanli Thank you for inviting me so quickly to your fork (you can remove me now), I don't know why sometimes I get blocked by authentication in PRs 😕

Copy link
Member

@lfades lfades left a comment

Choose a reason for hiding this comment

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

@seanli Thank you for your hard work on this example 🥇

@lfades
Copy link
Member

lfades commented Apr 22, 2020

@seanli I changed the readme instrutions to make them more detailed about what to do (similar to the cms examples), and also renamed the example to be with-magic, because magic alone may sound like a Next.js feature, instead of a service/library that can be used with Next.js

@seanli
Copy link
Contributor Author

seanli commented Apr 22, 2020

Ah gotcha - thanks so much for the help reviewing and updating it @lfades! Super excited for this and adding it to our tutorial! 😄Let me know what's next, I do see "This branch is out-of-date with the base branch" should I update the branch or do you got it from here?

@lfades lfades merged commit 91adb86 into vercel:canary Apr 22, 2020
@lfades
Copy link
Member

lfades commented Apr 22, 2020

Merged, I was waiting for the linter to be okay 😄

@seanli
Copy link
Contributor Author

seanli commented Apr 22, 2020

This is awesome! Thanks man, it was a lot of fun ⭐️

@vercel vercel locked as resolved and limited conversation to collaborators Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants