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

Allow the verify function to be a promise #142

Open
franciscop opened this issue Aug 5, 2021 · 0 comments
Open

Allow the verify function to be a promise #142

franciscop opened this issue Aug 5, 2021 · 0 comments

Comments

@franciscop
Copy link

franciscop commented Aug 5, 2021

Right now with all the modules that use Passport OAuth2, the verify function (2nd parameter of a Strategy) has this schema and workflow, when using callbacks:

const github = new GithubStrategy({...}, function verify(accessToken, refreshToken, profile, cb) {
  findUser(profile, function (err, user) {
    if (err) return cb(err);
    if (user) return cb(null, user);
    else createUser(profile, function (err, user) {
      if (err) return cb(err);
      cb(null, user);
    });
  });
});

I'd like to propose to allow for this verify function to be called asynchronously, and if so use promises instead of callbacks. This can be achieved two ways:

  • Checking the output of verify(...), and if it has a .then() you can assume it's a promise and don't work that way. That should be backwards incompatible though, since some people might be defining it as a promise anyway. See example below.
  • Checking for verify.length, which should give us how many parameters it expects and if it doesn't expect a callback treat it as a promise. We can add some further checks here (defining a .then(), that the value it's returned is not null, etc) but overall that's the idea.

A middle point can be reached now by doing some hacky way, that makes the first solution invalid since it'd not be compatible:

const github = new GithubStrategy({...}, async function verify(accessToken, refreshToken, profile, cb) {
  try {
    let user = await findUser(profile.id);  // Sample code
    if (!user) user = await createUser(profile));
    cb(null, user);
  } catch (error) {
    cb(error);
  }
});

So with the new proposed API, this would also be valid:

const github = new GithubStrategy({...}, async function verify(accessToken, refreshToken, profile) {
  let user = await findUser(profile.id);  // Sample code
  if (!user) user = await createUser(profile));
  return user;
});

Example of how it could be handled: https://jsfiddle.net/franciscop/qdu8fa79/

Edit: I can try to work on this with a bit of guidance, first and foremost is to know if this is the direction where passport-oauth2 wants to move forward, and second whether my 2nd proposed method is the wanted one by the project maintainers.

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

No branches or pull requests

1 participant