Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

Remove the need to verify the token per request #17

Merged
merged 4 commits into from
Feb 24, 2020
Merged

Remove the need to verify the token per request #17

merged 4 commits into from
Feb 24, 2020

Conversation

IamFlowZ
Copy link
Contributor

Description

This small change enables the user to optionally set the JWT_SECRET environment variable.

Related Issue

Support for federated services

Motivation

I'm working on a project that leverage's AWS Cognito for user identities, and so I don't have access to the original key that is used to generate the token. It would be nice if there was functionality that allowed for tokens to be accepted even if they are unverified within this instance.

@IamFlowZ IamFlowZ changed the title Feature/tokens from others Remove the need to verify the token per request Feb 11, 2020
@IamFlowZ
Copy link
Contributor Author

Any chances of this getting merged in soon?

@johnymontana
Copy link
Contributor

@IamFlowZ Thanks for the PR!

I like the idea but I'm a little concerned that with this approach a user could mistakenly forget to set the JWT_SECRET environment variable and then would be susceptible to vulnerabilities where the jwt would not be verified.

How about if we required another environment variable to be set, like JWT_NO_VERIFY or something to explicitly indicate the JWT will not be verified? Then the logic would be something like

 if (!JWT_SECRET && JWT_NO_VERIFY) {
      return jwt.decode(id_token)
    } else if (!JWT_SECRET) {
      throw new Error(	 
        "No JWT secret set. Set environment variable JWT_SECRET to decode token."	 
      ); 
    }
   else {
      return jwt.verify(id_token, JWT_SECRET, {
        algorithms: ["HS256", "RS256"]
      });
    }

@mohamedGamalAbuGalala
Copy link
Contributor

mohamedGamalAbuGalala commented Feb 20, 2020

@johnymontana
I don't like that we need to provide another secret to handle this issue instead, we check the type of the JWT_SECRET

if (typeof JWT_SECRET === 'boolean') {
  // check the value if false return jwt.decode(id_token)
} else {
  // normal stuff like
  if (!JWT_SECRET) {
    throw new Error(
      'No JWT secret set. Set environment variable JWT_SECRET to decode token.'
    );
  }
}

This way if we want to ignore the decoding we just set the same env variable to false.
And it's a must to put the JWT_SECRET.

@IamFlowZ
Copy link
Contributor Author

I agree that a flag plus the value is the better way to go. I think the typeof would be more negotiable, if we were working with pure javascript values. In node, unset environment variables return undefined. While undefined is still falsy it's probably better design to provide an explicit switch rather than an implicit type check. I will try to add this thought in this weekend. Thanks for the feedback :D

@IamFlowZ
Copy link
Contributor Author

@johnymontana I've added that environment variable as well as a bit at the end of the readme about how to run the tests locally so as not to eat up circleci credits. If there's any other changes you'd like to set let me know. Thanks!!

Copy link
Contributor

@mohamedGamalAbuGalala mohamedGamalAbuGalala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 👏

@johnymontana
Copy link
Contributor

Great - thanks!

@johnymontana johnymontana merged commit 9e663f2 into grand-stack:master Feb 24, 2020
@IamFlowZ IamFlowZ deleted the feature/tokensFromOthers branch February 24, 2020 17:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants