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

0.8 client decode jwt #314

Merged
merged 6 commits into from Oct 16, 2016

Conversation

Projects
None yet
4 participants
@marshallswain
Copy link
Member

marshallswain commented Oct 14, 2016

This adds a couple of util methods:

  • payloadIsValid returns true if the payload hasn't expired.
  • decodeJWT takes a token and returns the payload if it was valid.

getJWT has also been updated to only return the token if the token payload is valid.

Finally, decodeJWT is added to the app object, so you can do app.decodeJWT(app.get('token')) to get the token payload. It's the same as the decodeJWT util, so it only returns a payload if it was valid.

Includes tests. Will need docs before release:

marshallswain added some commits Oct 13, 2016

Only set app token if it’s not expired.
Modifies the getJWT function to return undefined if the token has expired.  This gives you a quick way of knowing if the user is (more than likely) authenticated without having to make a request.
Allow client to decode token.
This adds an `app.decodeJWT` function that decodes a token synchronously.  With a non-expired token, it returns the token payload, otherwise it returns undefined.

Will need docs before publishing 0.8 or whichever version we’re doing next.
@daffl

This comment has been minimized.

Copy link
Member

daffl commented Oct 14, 2016

Should we make it the same as on the server where we now have

app.authentication.options // authOptions
app.authentication.verifyJWT // decode + verify
app.authentication.getJWT // get token

For the server app.authentication is an instance of this class: https://github.com/feathersjs/feathers-authentication/blob/0.8/src/base.js

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Oct 14, 2016

Definitely. I'll update it tomorrow.

@marshallswain marshallswain self-assigned this Oct 16, 2016

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Oct 16, 2016

@feathersjs/core-team do we want to keep the logout method where it is at app.logout(), or move it to app.authentication.logout()?

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Oct 16, 2016

hmm. I think keep it where it's at. We have app.authenticate as well. Which might be better to be app.login so that people don't get confused.

@slajax

This comment has been minimized.

Copy link

slajax commented Oct 16, 2016

I like having alias easily accesssible on app. That said, its more important to me that it is consistent if there is a client / server implementation.

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Oct 16, 2016

Ok, so we'll have.

app.authenticate()
app.logout()
app.authentication.options // authOptions
app.authentication.verifyJWT() // decode + verify
app.authentication.getJWT() // get token
@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Oct 16, 2016

Ya let's go with that. Once you get those changes in @marshallswain ping one of us for a quick ship. I'd like to get this in ASAP so that we can hopefully wrap up auth in the next couple days and move on to crushing the CLI/Generators

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Oct 16, 2016

Will do. Also, I'm making both verifyJWT and getJWT sync, which won't match the server API (where both return promises), but I think promises will unnecessarily complicate things. Let me know if you feel different.

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Oct 16, 2016

@ekryski we're not returning the user in the authenticate requests anymore, right? So this stuff should be able to come out, I think: https://github.com/feathersjs/feathers-authentication/blob/0.8-client-decode-jwt/src/client/index.js#L54

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Oct 16, 2016

@marshallswain correcto. Will have to be a separate request by the client explicitly because you may not be authenticating a user. Could be a device, organization, toaster, whatever...

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Oct 16, 2016

Will do. Also, I'm making both verifyJWT and getJWT sync, which won't match the server API (where both return promises), but I think promises will unnecessarily complicate things. Let me know if you feel different.

I don't see a problem with that but maybe @daffl wants to weigh in.

ekryski and others added some commits Oct 14, 2016

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Oct 16, 2016

Other than what we discussed above, the only additional change is that verifyJWT now allows passing an object with a token property on it, which matches the server. This should be ready to ship unless anybody finds any issues. If not, please pull the trigger and release. I'm retiring for the night. 🌙 😴

@@ -3,3 +3,5 @@ node_js:
- 'node'
- 'iojs'
- '0.12'
code_climate:
repo_token: 02ea69f85221efd3051a19e882c87cebc07522bf321374186bb6116301569e5e

This comment has been minimized.

@ekryski

ekryski Oct 16, 2016

Member

This is slightly different to get it working but I will update.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Oct 16, 2016

Nice! You pulled out the permissions stuff too. :shipit:

@ekryski ekryski merged commit 75e971f into 0.8 Oct 16, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ekryski ekryski deleted the 0.8-client-decode-jwt branch Oct 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.