Zaubrik / djwt

Create and verify JSON Web Tokens (JWT) with Deno or the browser.
MIT License
225 stars 23 forks source link

How to extract payload #1

Closed sarthaksavvy closed 4 years ago

sarthaksavvy commented 4 years ago

I have created this function to extract the payload

async fetchPayload(token: string):Promise<object> {
    const data = await validateJwt(token, key, { isThrowing: false });
    return data.payload;
  },

but I am getting this error

TS2322 [ERROR]: Type 'Payload | undefined' is not assignable to type 'JwtObject | null'.
  Type 'undefined' is not assignable to type 'JwtObject | null'.
      return data.payload;
timonson commented 4 years ago

Hi @sarthaksavvy, I would recommend checking data for null. The return type of validateJwt is Promise<JwtObject | null>. Please let me know if the following code works:

async fetchPayload(token: string):Promise<object | null> {
    const data = await validateJwt(token, key, { isThrowing: false });
    return data ? data.payload : null;
  }

I am considering changing the return type of validateJwt to Promise<JwtObject> in the future. Before doing that I would love to get some feedback about the question if the option isThrowing is used by the community.

dev-err418 commented 4 years ago

Hi, to answer your question above, yes I'm currently using isThrowing. Hope it helped.

halvardssm commented 4 years ago

Hi @timonson ! I am using your library in an Oak middleware that I have made for JWT validation.

I would suggest to have isThrowing default as false.

I am using isThrowing to be able to return custom error messages and to have custom error validation. However, I think a better way of handling this is to return JwtObject | undefined instead of throwing and JwtObject | null. This is in line with the conventions of ts/js. This way the user can handle error handling however they see fit. If the JWT is valid but the token is expired, the property isExpired could be included in the data.

Hope this helped!

timonson commented 4 years ago

Hi @halvardssm, I am definitely open to change the default of isThrowing to false. But could you please elaborate why it should be JwtObject | undefined instead of JwtObject | null. I did a little bit of research regarding JS conventions before and I derived the opposite from that, meaning null would be a better fit than undefined.

Thank you!

halvardssm commented 4 years ago

Sure!

As stated in the TypeScript coding guidelines, one should not use null but rather undefined. I also think it is more natural to use undefined as a return value since this is the default for when a value is missing in TS/JS, and null is not widely used in the JS world in general (it's an awkward type to deal with).

I can't argue to a greater extent to why use one over the other since ultimately it depends on the conventions you follow, but I hope this helps.

timonson commented 4 years ago

@halvardssm What are your thoughts about the following return values of validateJwt? Do the changes improve it from your perspective? Thank you!

type JwtObject = { header: Jose; payload?: Payload; signature: string };
type JwtValidation =
  | (JwtObject & { isValid: true })
  | { jwt: unknown; error: JwtError; isValid: false; isExpired: boolean };

async function validateJwt(
  jwt: string,
  key: string,
  { critHandlers }: Opts
): Promise<JwtValidation> {
  try {
    const [oldJwtObject] = await handleJwtObject(
      validateJwtObject(parseAndDecode(jwt)),
      critHandlers
    );
    if (
      oldJwtObject.signature !==
      parseAndDecode(makeJwt({ ...oldJwtObject, key })).signature
    )
      throw Error("signatures don't match");
    return { ...oldJwtObject, isValid: true };
  } catch (err) {
    return {
      jwt,
      error: new JwtError(err.message),
      isValid: false,
      isExpired: err.message === "the jwt is expired" ? true : false,
    };
  }
}
halvardssm commented 4 years ago

@timonson I think this is a really good implementation 👍 I would maybe change the validation type to the example below to always return the JwtObject in the jwt prop, but I think both ways have their pros and cons so I leave it to you:

type JwtValidation =
  | { jwt: JwtObject; isValid: true }
  | { jwt: string; isValid: false; isExpired: boolean; error: JwtError };
timonson commented 4 years ago

I hope this commit https://github.com/timonson/djwt/commit/289b373ece01aebe1322d2cb4ad97afcfdb52ea3 addresses this issue, thanks to all!