nextauthjs / next-auth

Authentication for the Web.
https://authjs.dev
ISC License
24.05k stars 3.33k forks source link

Allow using custom Passport LocalStrategy for local authentication #18

Closed ghost closed 6 years ago

ghost commented 6 years ago

This is related to https://github.com/iaincollins/next-auth/issues/9 as it is about doing local authentication, but it is about using a custom Passport LocalStrategy I have written. Is it possible to use Passport LocalStrategy with next-auth? I tried creating a provider and passing the strategy in the Strategy property, but that wouldn't work. I wonder if I am doing something wrong. Does it only work with OAuth providers?

providers.push({
  providerName: 'Local Login',
  providerOptions: {
    scope: ['profile', 'email'],
  },
  Strategy: require('./passport/local-login'),
  strategyOptions: {
  },
  getProfile(profile) {
    // Normalize profile into one with {id, name, email} keys
    return {
      id: profile.id,
      name: profile.displayName,
      email: profile.emails[0].value,
    };
  }
});
iaincollins commented 6 years ago

The current system is only designed to support oAuth explicitly and doesn't provide support for LocalStrategies, although it's a reasonable request I'd be happy to accommodate in future.

For now, if you just want to support logging in with a username and password you can add a route that calls req.login() and does something like this:

expressApp.get(`/auth/password`, (req, res) => {
  UserCollection.find({ email: req.body.email })
  .then(user => {
    if (user && user.password = encryptPassword(req.body.password)))
      req.login(user, (err) => {
        return res.redirect(`/auth/callback?action=signin&service=password`)
      })
    } else {
      return res.redirect(`/auth/error?action=signin&type=password-failure`)
    }
  })
  .catch(err => {
    return res.redirect(`/auth/error?action=signin&type=password-error`)
  })
})

If you'd like to see an example of an index.js that that invokes NextAuth and sets up custom routes after invoking it take a look at this example.

Note: Be sure not to pass a 'port' to NextAuth if you want to add custom routes. As in the example you will have to define default routing for Next.js and call expressApp.listen() after you have finished declaring any custom routings.

For some background, this is what the email token sign in feature does and will give you exactly the same behaviour (and so you will be able to also support linking with oAuth accounts too, if you like).

I am happy to look at making this easier in future through direct support for localStrategies - and also looking at adding the ability to turn off email only sign in, perhaps in favour of some sort of user registration option, if that would make more sense for some people.

Generally email token based sign in (as used by sites like Slack and Medium) is regarded as good practice from a security perspective - even if the implementation in NextAuth is currently a bit simple (the tokens are single use, but don't expire) - as it prevents the possibility that even encrypted passwords might be leaked (because they are all transitory and single use).

So, I'm included to encourage to people to use that option by default - but I appreciate it's not always suitable for all use cases and am happy to look at supporting other scenarios, including things like localStrategies that 2FA.

ghost commented 6 years ago

@iaincollins I did this, added a password field to the signin component, and updated my form action URL to /auth/password and it works with JavaScript disabled, but when JavaScript is enabled the call to NextAuth.signin() in handleSubmit() ends up sending the verification email anyway, I think because next-auth-client does the POST to /auth/email/signin. Should I not call NextAuth.signin()? Can I use next-auth/next-auth-client when wanting to have password authentication?

I understand the single-use login token links sent via email can be seen as more secure than keeping passwords, but I am in an organization that precludes me from doing it that way.

Support for password-based authentication would be great, but I really just want to get this working the way I need. I had considered rolling back and modifying the code from before you moved it to the next-auth module, but I am so far down the line now there would be a lot to do over again.

iaincollins commented 6 years ago

To confirm, you don't call NextAuth.signin(), just post to the new end point.

You can do this using just a normal form post and/or in JavaScript.

You don't actually also need to do it in JavaScript unless you want to - a normal form post will work just fine, but if you want to you could have a sign in method like this in your sign in page (assumes email and password are stored in 'this.state').

import React from 'react'
import Router from 'next/router'
import { NextAuth } from 'next-auth/client'

export default class extends React.Component {
  constructor(props) {
    super(props)
    this.state = {
      email: '',
      password: ''
    }
    this.handleSignin = this.handleSignin.bind(this)
  }

  handleSignin() {
    // Make sure we include the latest CSRF Token in the POST request as _csrf
    const formData = {
      _csrf: await NextAuth.csrfToken(),
      email: this.state.email,
      password: this.state.password
    }

    // Encoded form parser for sending data in the body
    const encodedForm = Object.keys(formData).map((key) => {
      return encodeURIComponent(key) + '=' + encodeURIComponent(formData[key])
    }).join('&')

    return fetch('/auth/password', {
      method: 'POST',
      headers: {
        'Content-Type': 'application/x-www-form-urlencoded'
      },
      body: encodedForm,
      credentials: 'same-origin'
    })
    .then(response => {
      if (response.ok) {
        Router.push(`/auth/callback}`)
      } else {
        Router.push(`/auth/error?action=signin&type=password&email=${this.state.email}`)
      }
    })
  }

}

If you find this works for you I am happy to look at both adding localStrategy support and at changing NextAuth.signin() to work with them (so it can be passed options to post to a local strategy, e.g. username or email, password, 2-factor-auth-token, etc).

None of these changes should be breaking - so anything you have done will keep working! - but should make it easier for anyone else in future.

Jexah commented 6 years ago

Hi, sorry to be a pain. I'm working on an application in which I require local authentication, with restricted login access. I haven't implemented it yet, but it looks like the above would satisfy the requirement for password authentication. My concern is regarding a case in which the client posts to /auth/email/signin, and has an account created for them via the default behavior. I don't want to maintain my own branch, and so I don't want to bastardize the source by ripping out and replacing behaviors. I attempted to override the route, but it seems like this is impossible without modifying the source.

My guess as to the best way to achieve this without branching the library is to define the sendSignInEmail function in next-auth.functions.js as an empty function. That way, at least the email won't send, but unfortunately the account is still created. Is this the best possible approach?

Please note that my concern is not with disabling the email sign in, it is simply the email sign in creating user accounts. Disabling email sign in is simply one method of preventing anonymous (or even non-administrator) users of the site from creating accounts.

Aside: What is your guestimated timeline on built-in password authentication?

Edit: I was able to disable the email sign in by using the expressApp property that nextAuthOptions uses to hook into an existing Express instance. First I defined an Express instance, set the route to be handled by Next.js, passed in the Express instance into nextAuthOptions, and voila! Email sign in disabled! :D

.then(nextAuthOptions => {
  // Pass Next.js App instance and NextAuth options to NextAuth
  // Note We do not pass a port in nextAuthOptions, because we want to add some
  // additional routes before Express starts (if you do pass a port, NextAuth
  // tells NextApp to handle default routing and starts Express automatically).
  nextAuthOptions.expressApp = Express();
  const pathPrefix = nextAuthOptions.pathPrefix || '/auth';
  nextAuthOptions.expressApp.post(`${pathPrefix}/email/signin`, (req, res) => {
    let nextRequestHandler = nextApp.getRequestHandler()
    return nextRequestHandler(req, res)
  });
  return nextAuth(nextApp, nextAuthOptions)
})

Edit 2: Using the above information you provided I was able to get the password authentication working. Thanks!

iaincollins commented 6 years ago

Hi @Jexah, not at all - requests for specific things are great.

I will try add the ability to define a localStrategy and not use email sign in route this weekend.

Thanks for sharing this! :-)

Jexah commented 6 years ago

Wow, that is fast! Thanks for the update! Quick question, unrelated and more out of curiosity than anything else, why don't you use native async/await instead of promises?

ghost commented 6 years ago

@Jexah I had the same question. Maybe it's that async/await are not as flexible? A talented longtime JavaScript programmer is probably better versed with .then() style promise handling. Personally, I would prefer async/await having a synchronous programming and C# background.

@iaincollins I eagerly await the local strategy example! I have not yet had time to try what you suggested, so if you end up writing that example first, it should help me out greatly and I would greatly appreciate it.

iaincollins commented 6 years ago

Hmm, that's a good question.

I suspect I probably had ideas around error handling with promises that have never been implemented, or other vague ideas about maybe executing other things concurrently while waiting for the response that were never followed up on.

Some of the updated client code was also gratefully received via a PR - though this may not actually be reflected in the git commits has had to quickly revert and then re-add the code in a preview version after it surfaced a bug that was since quashed.

The code existed in a starter project repo before it was forked off, and I think some it may predate async/await support in Next.js. The flip side of this is it means it's had time to iron out the kinks but some of it might do with refactoring.

Please do feel free to raise issues where you think the code could do with a review!

Jexah commented 6 years ago

Promises haven't been around that long. Async/await uses promises under the hood, and you can do anything with async/await that you can do with promises (maybe not natively, I'm on my phone right now so I can't be bothered checking, but last I checked there was no native awaitAll). I converted the next-starter project to async/await, built a proper database interface, did the auth stuff I mentioned above, converted from bootstrap to material-ui, and started using Node 9 mjs modules. So far I'm loving it!

Do you accept pull requests on either repo? You can certainly run things concurrently with async await and (I think) the error handling is much nicer with regards to thread safety, though you need to make sure you catch everything, as you do with promises.

An example of executing stuff while waiting for some response from async function.

async function func (){
  return (await doStuff).map(a => a+1);
}
...
async (req, res) => {
  let promise = func(); // Start executing `func` and assign returned Promise variable.
  // start processing something else / do some sync stuff
  ...
  // Nothing left to do, so we send the response.
  res.send (await promise); // This will return the value passed into the `result` parameter of the promise.
}

In comparison:

function func (){
  return doStuff.then(result => {
    resolve(result.map(a => a+1));
  });
}
...
(req, res) => {
  func().then(res.send);
  // start processing something else / do some sync stuff
  ...
  // Nothing left to do.
}

At least one difference between these that I have left in for simplicity, is that you cannot guarantee that the sync stuff in the middle has been completed before res.send in the second example, but you can in the first. This would be solved by wrapping the sync code in a Promise, and waiting for both the func() Promise and sync Promise to be resolved before running res.send.

@kelleg1 PM or email me or something, I'm happy to discuss details further.

ghost commented 6 years ago

@Jexah Sounds cool. We probably ought not derail this topic from the conversation about local strategies, but something I am facing needing to do also is to use Mongoose with multiple connections. Since Mongoose uses the singleton pattern and only supports one connection per Mongoose instance, I have to use multiple instances if I want to use it. I do like it over the native driver. I managed to do this with some other projects, so hopefully I won't have too much trouble integrating it into Next.js Starter.

I'd like to see your code. I was looking into Material UI too, but currently I use Reactstrap in addition to making my own components.

iaincollins commented 6 years ago

Hi, thanks for the feedback, agree it's best to move other discussions into another thread.

To confirm, I do know how await/async and Promises work.

The issue was that await/async was not originally supported in older versions of node (for the server side code) and was not originally transpiled by babel in older versions of Next.js (for the client side portions of the code). This is no longer an issue in either case though.

If you use async/await do note they are not quite the same. Code in a function after an await statement will not run until after the await call has returned - this is unlike with a Promise, where code after a Promise in the same function will run without waiting for the Promise to return.

This is mostly not a problem in NextAuth but can change the behaviour of an app as when used it forces all code to execute serially, rather than allowing asynchronous calls to happen in Parallel.

I do always try to accept pull requests, but there aren't any unit tests for the auth yet so maybe best not to refactor any of the more sensitive stuff - specifically the client as it requires more testing, mostly to make sure Internet Explorer is happy, as it's more prone to having problems.

With regard to Mongoose, both the current version of Mongoose and the underlying Mongo driver use singletons (object caching in require) and connection pooling. The default for both is 5 connections and you can use the poolSize option in either to change it.

Jexah commented 6 years ago

I totally get the whole lack of support thing. With regards to the rest of your post: Functionally, async/await and Promises are exactly the same, but the implementation is just slightly different. I think while I was discussing functionality you thought I was talking about implementation, in which they are different. If you were discussing implementation, please disregard the rest of this comment, but if you are saying that they functionally differ, please read the following.

While what you say regarding await is correct, the way you are implying it should be implemented is incorrect. Rarely in actual implementation do you do something like await asyncFunc(), as that does indeed block execution. Typically, you rarely -- if ever -- await a Promise until the value is absolutely required, as in the example I provided above. First I begin execution of an async function, and assigned the returned Promise to the promise variable, which is literally a native Promise. An async function is just a native wrapper for a function that returns a Promise, and await waits for resolution of the Promise. This means you can await a native Promise or someAsyncFunction().then(res => { ... }).

tl;dr async and await absolutely do not force code to run serially. They are just native syntactic sugar to make handling Promises nicer to read and write, and by extension, using them doesn't change the core underlying functionality of Promises, and thereby cannot have different functionality to that available in native Promises.

Cheers for looking into password authentication! Pretty sure this is the only starter project that ships with Nextjs authentication working (with proper security) OOTB.

iaincollins commented 6 years ago

Hey @kelleg1, I've just published 1.8.0 to NPM which addresses this.

There is now the option to define a signIn() method in next-auth.functions.js. It's easy to consume and there is an example of how to use it in the front end on example/pages/auth/credentials.js.

If you define a signIn() method it enables POST requests to /auth/signin. You can pass any options you like to it by passing an object to NextAuth.signin().

Additionally, if you want to disable email based sign in, just don't define a sendSignInEmail() function (or set it to null) and NextAuth will now longer set the routes for them (so people won't be able to create accounts unless you provide a specific endpoint outside of NextAuth for them to do that).

tcash11 commented 6 years ago

@iaincollins You mention in your previous post that

There is now the option to define a signIn() method in next-auth.providers.js.

Did you mean next-auth.functions.js? or is there something we need to add in the providers file as well for a username/pw type local strategy?

iaincollins commented 6 years ago

Sorry, yes thanks for the correction:

next-auth.functions.js and auth/credentials.js contain example usage.

I know it's not especially well documented, but I hope to improve it and the documentation for it.