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

Revisit the passport integration #237

Open
mlaccetti opened this issue Apr 7, 2018 · 1 comment
Open

Revisit the passport integration #237

mlaccetti opened this issue Apr 7, 2018 · 1 comment

Comments

@mlaccetti
Copy link
Member

Right now, each authentication mechanism must have a completely separate implementation, which also makes it difficult to track down issues if something goes wrong during authentication, as Ravel itself doesn't know about it. Most auth providers follow the same approach:

app.get(this.ravelInstance.get('auth path'), passport.authenticate('this-provider'));

app.get(this.ravelInstance.get('auth callback path'),
  passport.authenticate('this-provider', {
    failureRedirect: this.ravelInstance.get('login route'),
    successRedirect: this.ravelInstance.get('app route')
  })
);

We should be able to implement all of this functionality internally, and have the authentication provider expose portions that can be overridden when required. If the redirect functionality is not 100% across all providers, Ravel can also leverage passport's custom callbacks instead of mandating redirects (we'll have to figure out how to give the function the proper context - req/res/..., as required):

app.get(this.ravelInstance.get('auth callback path'),
  passport.authenticate('this-provider', function(err, user, info) {
    if (err) { return Promise.reject(err); }
    if (!user) { return Promise.resolve({redirect: '/login'}); }
    req.logIn(user, function(err) {
      if (err) { return next(err); }
      return res.redirect('/users/' + user.username);
    });
  })(req, res, next);
  })
);
@mlaccetti mlaccetti added this to the Release 1.0.0 milestone Apr 7, 2018
@Ghnuberath
Copy link
Member

I think it makes sense to leave AuthenticationProvider as-is, but to extend it to another abstract superclass which encompasses this functionality. That way AuthenticationProvider creators can extend either as they see fit.

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

No branches or pull requests

2 participants