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

Naming of functions generating JWT, and consistency of data associated with JWT #3

Open
polymorpher opened this issue Nov 20, 2024 · 3 comments

Comments

@polymorpher
Copy link

In

async getTokens(payload: object) {
, the function is named as a get-only function (i.e. pure), but in fact it mutates the server state, generate and records a JWT token at the server (via jwtService.signAsync). The name should be updated to reflect that, e.g. create...

In

const userData = this.jwtService.verify(body.token);
, a new token is generated using returned userData with a few JWT fields deleted. This can be potentially inconsistent with the payload given in other places, such as
const jwt = await this.userService.getTokens(payload)
. It could lead to error if new fields are added later one, or if library updates and assigns new fields to the payload

@ArtemKolodko
Copy link
Collaborator

ArtemKolodko commented Nov 22, 2024

  1. getTokens

Function doesn't make any mutations on the server side. signAsync creates object in memory and return the resulted object (accessToken / refreshToken).
Generated tokens are not stored anyware on the backend side and are passed directly to the client

@ArtemKolodko
Copy link
Collaborator

ArtemKolodko commented Nov 22, 2024

  1. this.jwtService.verify

Agree, theoretically, deleting can be dangerous, but iat, exp and jti is a service properties described in specification:
https://auth0.com/docs/secure/tokens/json-web-tokens/json-web-token-claims#registered-claims

Screenshot 2024-11-22 at 4 53 44 PM

It is very unlikely that these properties will be excluded or renamed.
About the payload - it's the dev responsibility; if you are adding new fields to JWT you have to understand how it works and check the specification at least once.

Current PumpOne JWT structure:
Screenshot 2024-11-22 at 5 00 42 PM

A solution can be to wrap the user payload (username, etc) into additional property, but it would result in a slight increase of JWT token size.

Considering all of this, I prefer to leave it as it is, but it's discussible of course! Thanks for pointing it

@polymorpher
Copy link
Author

Sounds good, let's get back to this when the structure becomes more dynamic and complex

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

No branches or pull requests

2 participants