ArkeologeN / node-linkedin

LinkedIn 2.0 wrapper in Node.js
MIT License
163 stars 78 forks source link

callback support for state setting and validation #91

Open joeauty opened 5 years ago

joeauty commented 5 years ago

This is a fix for the issue here:

https://github.com/ArkeologeN/node-linkedin/issues/56

It requires that users provide their own state setting and validation methods, e.g.:

function setLinkedinState(state, redirectURI) {
  return new Promise(function(resolve, reject) {
    CSRFToken.create({
      token:state,
      redirectURI:redirectURI
    })
    .then(function(results) {
      resolve(results);
    })
    .catch(function(err) {
      reject(false);
    })
  });
}

function validateLinkedinState(state) {
  var token;

  return new Promise(function(resolve, reject) {
    CSRFToken.findOne({
      token:state
    })
    .then(function(results) {
      if (results) {
        token = results;
        // delete token
        return CSRFToken.findOneAndRemove({
          token:state
        });
      }
      else {
        return Promise.resolve(true);
      }
    })
    .then(function(results) {
      resolve(token ? results : false);
    })
    .catch(function(err) {
      resolve(false);
    })
  });
}

Example calls:

Linkedin.auth.authorize(scope, null, process.env.LINKEDIN_CALLBACK, setLinkedinState);

(expects a function for setting state)

Linkedin.auth.getAccessToken(res, req.query.code, validateLinkedinState(req.query.state))

(expects a promise resolving in a true/false validation)

The fact that the stateValidation method expects a promise is probably a little problematic in this current codebase since it is callback based, but I know there are plans to move this library over to promises, and I was having difficulties getting this to work with my chosen adapter (Mongoose) since this ORM doesn't seem to have a way to not return promises with function calls. So, I expect this might warrant some conversation?

This also assumes that everybody will care about HA (it does away with the old memory-based state tracking completely). I couldn't really settle on a clean way to support both, and I figured that most developers would at least want to future-proof their code for HA, so this breaks backwards compatibility. I know that there is a 2.0 codebase in-the-works too, so I decided not to worry about that for now since it might make sense to implement this feature in the 2.0 codebase, which will obviously break backwards compatibility anyway.

ArkeologeN commented 4 years ago

@joeauty - I just saw it and it seems I overlooked it somewhere. Shall I still merge it? Would you like to be a core contributor?

joeauty commented 4 years ago

@joeauty - I just saw it and it seems I overlooked it somewhere. Shall I still merge it? Would you like to be a core contributor?

I'm no longer working on this project and probably won't have additional time to make contributions, but I'm happy to help you get this merged if you think it will add some value to the project!