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

Example: With cookie authentication #3955

Closed
wants to merge 14 commits into from

Conversation

malixsys
Copy link
Contributor

@malixsys malixsys commented Mar 6, 2018

This adds an example using cookies for authentication

c.f. #153

@malixsys malixsys changed the title With cookie auth Example: With cookie authentication Mar 8, 2018
@malixsys
Copy link
Contributor Author

@timneutkens what is the review process?

@malixsys
Copy link
Contributor Author

CC @sergiodxa, ... ?

{user.email
? (
<React.Fragment>
<li onClick={processLogout} style={{ textDecoration: 'underline', cursor: 'pointer' }}>Logout</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

A <button> with a click handler should be used instead for proper semantics and better accessibility.

export const processLogin = async ({ email, password }) => {
const user = await axios
.post('/api/login', { email, password })
.then(response => response.data)
Copy link
Member

Choose a reason for hiding this comment

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

since you're using async, you can write this with async :

const response = await axios.post('/api/login', { email, password })
const user = response.data

}
}

export const processLogout = () => {
Copy link
Member

Choose a reason for hiding this comment

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

maybe you can do this async ?

export const processLogout = async () => {
// ...
await axios.post('/api/logout')
Router.push('/login')

return { auth }
}

export const getProfile = () => axios
Copy link
Member

Choose a reason for hiding this comment

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

maybe you can do this async ?

export const getProfile = async () => {
  const response = await axios.get('/api/profile')
  return response.data
}

@jimthedev
Copy link

I like this example. I was hoping to adapt it for auth0 but not sure where it is at. Any updates?

@malixsys
Copy link
Contributor Author

malixsys commented Jul 25, 2018

I’ll update it. Not sure how the PR review / merge process works... i.e. who merges it to master?

@jimthedev
Copy link

I think it has to go into canary first? Seems like @timneutkens is the one doing that regularly so maybe he will have feedback?

@jsardev
Copy link

jsardev commented Aug 7, 2018

Hmm... But what happens when I'll manually set window.__USER__ = { type: 'authenticated' }? Will it not allow me to access the secured page while being on the client-side?

@malixsys
Copy link
Contributor Author

malixsys commented Aug 7, 2018

@sarneeh still secure as the server looks for a signed cookie

https://github.com/zeit/next.js/pull/3955/files#diff-59c49209a4ae9741eb402e16c3fbd1b4R61

@jsardev
Copy link

jsardev commented Aug 7, 2018

@malixsys Yeah, my bad, didn't noticed this 😄

@malixsys
Copy link
Contributor Author

malixsys commented Aug 7, 2018

@timneutkens how does this get merged?


// fake serverless authentication
const authenticate = async (email, password) => {
const users = await axios.get('https://jsonplaceholder.typicode.com/users').then(response => response.data)

Choose a reason for hiding this comment

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

Using https cause error in development. Better use something like const scheme = dev ? 'http' : 'https'?

return res.status(200).json(info)
})

server.post('/api/logout', (req, res) => {
Copy link
Member

Choose a reason for hiding this comment

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

The API should be separate from the Next.js server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be. On sites of a certain size, it probably should be.
This is an example. This way, there is no need to fire another server and no CORS problems...


server.use(express.json())

server.use(cookieParser(COOKIE_SECRET))
Copy link
Member

Choose a reason for hiding this comment

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

Cookieparser shouldn't be used as it's an express middleware, instead use something like npmjs.com/next-cookies to parse the cookie in getInitialProps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timneutkens Should the example be renamed?
This uses Express server-side to handle the Auth for demonstration purposes...

.then(() => {
const server = express()

server.use(express.json())
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be used, I assume it won't be needed when moving out the API into it's own server.


const redirect = (res, path) => {
if (res) {
res.redirect(302, path)
Copy link
Member

Choose a reason for hiding this comment

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

res.redirect is not something officially supported, it comes from Express, this should use the location header.

const redirect = (res, path) => {
if (res) {
res.redirect(302, path)
res.finished = true
Copy link
Member

Choose a reason for hiding this comment

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

res.finished is no longer needed if res.end is called

<Head />
<body>
<Main />
<script dangerouslySetInnerHTML={{ __html: getUserScript(user) }} />
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed, you can use _app.js and then assign to window.__USER__ in constructor of _app.js

Copy link
Member

Choose a reason for hiding this comment

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

Then _document.js can also be removed I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, no longer needed... This was written before _app I think... :)

@@ -0,0 +1,97 @@
import Router from 'next/router'
import axios from 'axios'
Copy link
Member

Choose a reason for hiding this comment

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

Let's use isomorphic-unfetch like the other examples

Copy link
Contributor Author

@malixsys malixsys Nov 15, 2018

Choose a reason for hiding this comment

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

I guess this example is too opinionated to be included. 🤔
It works with Express and Axios because they are widespread and I find them easier to work with, and the reader might, too.
At the time of its creation in March, I could find no other comprehensive yet concise example of how to do proper secure, cookie-based auth...
Not bitter, just debating how willing I am to complexify this example with a separate API server.
Axios IS no biggy to fix.

@timneutkens
Copy link
Member

@malixsys Invited you to the repository holding an auth example I started a while back: https://github.com/timneutkens/next.js-auth-example/invitations

@timneutkens
Copy link
Member

I'm going to close this PR in favor of #5821, thank you so much for your contribution though, I'm sure people have found this useful 🙏

@malixsys
Copy link
Contributor Author

No problem @timneutkens , I'll look at your invitation.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 11, 2019
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.

7 participants