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

callback support for state setting and validation #91

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

callback support for state setting and validation #91

wants to merge 3 commits into from

Conversation

joeauty
Copy link

@joeauty joeauty commented Mar 5, 2019

This is a fix for the issue here:

#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
Copy link
Owner

@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
Copy link
Author

joeauty commented Aug 5, 2020

@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!

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

Successfully merging this pull request may close these issues.

2 participants