adonisjs / auth

Official Authentication package for AdonisJS
https://docs.adonisjs.com/guides/auth/introduction
MIT License
195 stars 65 forks source link

Feature Request: Ability to serialize object into JWT Token Payload #13

Closed starr0stealer closed 7 years ago

starr0stealer commented 8 years ago

I wanted to start this conversation here before offering a pull request. I want to see what the community thinks as well as project maintainers.

When using Adonis Auth with JWT, the sample code below only serializes the Entities Primary Key into the Payload.

const user = yield User.find(1)
const token = yield request.auth.generate(user)

Code: https://github.com/adonisjs/adonis-auth/blob/master/src/Schemes/Jwt/index.js#L93-L103

JWT Spec and the jsonwebtoken library support providing object literals into the payload. It would be very useful if Adonis Auth also supported this use-case. An example of practical use of this feature would be such as if you want to add the Users Profile object into the payload, so you don't need to make another API call for that information (or and it to the response itself).

Without this feature you have to work around it by adding the object into the response body (to avoid another Web I/O).

const profile = yield user.profile().fetch();
const token = yield request.auth.generate(user);
response.send({ token: token, profile: profile });

Many client side frameworks have built in support for JWT payloads, example Aurelia Auth, which has an API to get the JWT payload. If the payload had an object expected you wouldn't need to do any further processing to get the details of the Users Identity, or any special logic to store and maintain that information through the life of the session, which is something the framework is already doing with the payload.

Offering this feature is trivial, there are a few different ways I see how it could be done, but first would like to here others thoughts. If anyone wants to see sample change-sets in how this could be achieved I would be glad to provide and discuss further

thetutlage commented 8 years ago

@starr0stealer How would you decode the payload on client side? since there is a secret associated with encoding the data.

starr0stealer commented 8 years ago

JWT is a defined standard that formats a token in 3 parts, Header Payload Signature. The Header states its JWT and the algorithm used to created it. Payload is used to transmit any data you want to pass over. Signature is the Hash that includes the Header Payload secret.

The spec defines that we can read Header Payload not Signature. For that reason we can't change a JWT Token and expect it work.

Client Side frameworks already read the token. Such as Aurelia Auth and auth0/jwt-decode.

This is what prompts the feature request, to expand the Payload sent, to be more useful.

Edit : adding links with information about JWT

This information includes more details in how JWT works, as well as reserve keywords in the Payload, which would be needed to be outlined in the documentation.

Also worth noting, jsonwebtoken already sets values into the Payload passed over. For example it will set iat Issued At, and exp Expiration Time (if expiresIn is provided). Values the developer wont need to put into the Payload, but rather use existing configuration of Adonis Auth.

https://jwt.io/introduction/ https://scotch.io/tutorials/the-anatomy-of-a-json-web-token

thetutlage commented 8 years ago

Okay that make sense. I have another question, which is more related to usability.

You talk about attaching extra data to the payload, let's think of it Profile, what will happen if profile gets updated? The payload will still have the old data.

Whereas with id in the payload, you are kind of forced to fetch the update profile every time.

Thoughts?

starr0stealer commented 8 years ago

The developer should still be responsible to manage State of the JWT Payload. Normally if the underlying data of a JWT Token changes the token should be renewed. So if the Payload changes, they need to handle how they like, renew token or just make an API call for the update.

To maintain how Session works with Adonis Auth the primaryKey attached during generate() should still be the Identity id, usually this would be the same object as Profile, sometimes not. The Payload would be separate from the primaryKey object.

starr0stealer commented 8 years ago

I'm also thinking about how to handle entry points into the existing Adonis Auth API on the generate() method.

A few different use cases I can already think of, based on Payload needs and Identity Model design.

thetutlage commented 8 years ago

Trying to digest the flow.

  1. I can attach arbitrary data to the token generation call.
  2. There should be refresh method on the JWT auth, which will regenerate a fresh token.
  3. Also refresh will take an extra parameter of arbitrary data which was sent along to the generate method.

Also I would like to nest the payload inside a different key for better separations of concerns. For example: When i decode the token it should something like

{
  "identifier": 1,
  "data": {}
}
thetutlage commented 8 years ago

Using a single parameter for 2 things is kind of wrong approach. generate(identity, serializeIdentity = true) and generate(identity, payload = { })

thetutlage commented 8 years ago

Ideally I believe serializeIdentity should do the job. If I want to attach the user profile to the payload, I can eagerload the relationship. Which will have the user profile automatically serialized to JSON.

const user = yield User.find(Id)
yield user.related('profile').load()

request.auth.generate(user, true)
starr0stealer commented 8 years ago

Sample very basic change to generate() to meet different use cases. Other methods would need to change, since the Payload and location of primaryKey has changed.

  * generate (user, serializePayload) {
    if (!user) {
      throw new NE.InvalidArgumentException('user is required to generate a jwt token')
    }
    const primaryKey = this.serializer.primaryKey(this.options)
    const primaryValue = user[primaryKey]
    if (!primaryValue) {
      throw new NE.InvalidArgumentException(`Value for ${primaryKey} is null for given user.`)
    }

    let payload = {
        primaryKey: primaryKey
    }

    if (serializePayload != null) {
      const payloadType = typeof(serializePayload)
      if (payloadType === "boolean") {
        payload.data = user;
      } else if (payloadType === "object") {
        payload.data = serializePayload
      }
    }

    return this._signToken(payload, this.jwtOptions)
  }
starr0stealer commented 8 years ago

"eagerload the relationship" Can indeed work, but another use case is where you want to pass non entity model data. A sparse model, with just needed values for example.

thetutlage commented 8 years ago

@starr0stealer The idea is to force the user to follow the right design, instead of doing anything and coming up the spaghetti code.

I cannot think of any situation, where the payload should have data not related to a user. If there is, then they should hit the API instead of making JWT token a way of passing data around.

Also this makes the process of refresh token simple, as we can easily refetch the relationships, instead of asking the user to pass all the custom data again on refresh.

starr0stealer commented 8 years ago

I think that has a few negative affects. Anyone can decode the JWT Payload. The Identity model could have the password, which now needs to be transient otherwise you have security issues. If you just allow relationship to get more details, you also now have bloated data passing in the line (the first object you don't need, and other info such as Dates). What someone puts in the token can change App to App.

I like the idea of built in Token Refresh, not everyone will need that. Some that do, may need to apply their logic. In my current App I am handling Refresh myself, wouldn't like to loose that ability. To keep the token small, I only pass Name, Location, Image, not the entire Profile model.

I like a framework to simplify common tasks, but not limit what I can do for Business Rules

thetutlage commented 8 years ago

I have different thoughts here. I believe libraries should do the common tasks and frameworks should enforce better design.

Regarding the data security, you can also define which fields to hide to serialized JSON by defining visible or hidden properties on your model.

class User extends Lucid {

  static get hidden () {
     return ['password']
  }

}
starr0stealer commented 8 years ago

I completely agree with frameworks handling common tasks and best practices, but not limitations. The JWT spec allows anything to be put into Payload, and any implementation of it should do the same. In regards to data transfer over the net, common practice is to limit how much you pass over. A large Model hierarchy just for three fields isn't the best option.

thetutlage commented 8 years ago

@starr0stealer

I have to map my mind over it, can u share a PR with refresh and generate method accepting the payload? Also I am against the usage of a single parameter for 2 things.

donald-slagle commented 8 years ago

I use JWT in .NET using the jwt-dotnet/jwt library and it allows me to specify any information I would like in the payload. I like having the flexibility to define what I need and what I don't. It takes a dictionary that allows me to specify the key and value. I know that the jsonwebtoken library is used under the covers and it allows you to specify what you would like. It would be nice to be able to send a js object to the generate token method to specify the key and values you would like to pass with the token.

generate(payload, exp)

This would be a nice API and allow good flexibility. I feel like when a framework attempts to limit a user for security concerns it can get in the way of usability.

generate({
   userId: 1,
   name: 'Some Name',
   email: 'someemail@test.com'
}, 72000);

That API seems like it would give me the same usability as using the library directly and give full control over the payload and expiration time. The expiration time would be nice to be able to define in the auth.js configuration file as well, but allowing an override in the generate method for a "rememberme" type functionality.

The payload could be run through some sort of serializer that ignores certain properties based on the model if the model is passed which would be fine as well. I think documentation in the auth section would be enough to inform users not to send the password. If the user has used JWT in the past this would be a no brainer.

starr0stealer commented 8 years ago

@thetutlage I will be glad to put something more detailed together into a PR

RomainLanz commented 8 years ago

@thetutlage This is what we discuss earlier about extending the payload of the JWT and allow us to extend the JWT Validator.

thetutlage commented 8 years ago

@RomainLanz That was related to extend the configuration, this is more related to the actual data being sent over the wire.

I am very much open to get the ideal implementation for the JWT and API tokens Auth, and I believe the usage of the framework in production apps will have more requirements over the time.

RomainLanz commented 8 years ago

We discuss about both 😛

Why not pass an object that is merged into the payload?

yield request.auth.generate(user, object)

screen shot 2016-08-30 at 09 17 03
starr0stealer commented 8 years ago

I have two starting points, outlined in the Branches below. The idea of Refresh may need further discussion. The Developer would need to say when a Token needs updated, calling generate () will update the token.

This Branch handles the basic need, to include the User into the Payload: jwt-payload

  * generate (user, serializeIdentity) {
    if (!user) {
      throw new NE.InvalidArgumentException('user is required to generate a jwt token')
    }
    const primaryKey = this.serializer.primaryKey(this.options)
    const primaryValue = user[primaryKey]
    if (!primaryValue) {
      throw new NE.InvalidArgumentException(`Value for ${primaryKey} is null for given user.`)
    }
    let payload = {identityId: primaryValue}
    if (serializeIdentity) {
      payload.data = user
    }
    return this._signToken(payload, this.jwtOptions)
  }

This Branch handles above and, allows the user to provide any Object into the Payload: jwt-serialize

  * generate (user, serializeIdentity) {
    if (serializeIdentity) {
      return generateCustom(user, user)
    }
    const primaryKeyValue = this.getPrimaryKeyValue(user)
    return this._signToken({identityId: primaryKeyValue}, this.jwtOptions)
  }

  * generateCustom (user, model) {
    const primaryKeyValue = this.getPrimaryKeyValue(user)
    const payload = {identityId: primaryKeyValue, data: model}
    return this._signToken(payload, this.jwtOptions)
  }
starr0stealer commented 8 years ago

I created #14 in the simplest way to capture all use cases, usage examples below.

// API
  * generate (user, customModel) {
    if (!user) {
      throw new NE.InvalidArgumentException('user is required to generate a jwt token')
    }
    const primaryKey = this.serializer.primaryKey(this.options)
    const primaryValue = user[primaryKey]
    if (!primaryValue) {
      throw new NE.InvalidArgumentException(`Value for ${primaryKey} is null for given user.`)
    }
    let payload = {identityId: primaryValue}
    if (customModel) {
      payload.data = customModel
    }
    return this._signToken(payload, this.jwtOptions)
  }

// Usage
const user = yield User.find(Id)
yield user.related('profile').load()

request.auth.generate(user) // payload { indentityId }
request.auth.generate(user, user) // payload { indentityId, data : user }
request.auth.generate(user, user.profile().fetch()) // payload { indentityId, data : profile }
request.auth.generate(user, {foo: 'bar'}) // payload { indentityId, data : {...} }
thetutlage commented 8 years ago

@starr0stealer

Make sure to call .toJSON on the the model, which will return the serialized json instead of the actual model instance.

So it can be

payload.data = typeof(customModel.toJSON) === 'function') ? customModel.toJSON() : customModel
starr0stealer commented 8 years ago

@thetutlage I've updated to check for toJSON, thank you

marines commented 7 years ago

Any progress on that?

starr0stealer commented 7 years ago

@thetutlage

We are starting to get more people interested in this feature request, as its a really import feature of JWT and as stated by @marines any implementation without the ability to define a Payload doesn't provide much usage

thetutlage commented 7 years ago

@starr0stealer @marines I am really sorry, this issue has been on sideline for a while. I will add this feature tomorrow morning itself.

Have some more changes in mind with the proposed PR.

thetutlage commented 7 years ago

Done in https://github.com/adonisjs/adonis-auth/commit/2e413fe8643141f4d3752f4df04bd936bd65638d