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

Nested relationship data missing in JWT token and req.user #1361

Closed
dsod opened this issue Nov 10, 2022 · 2 comments
Closed

Nested relationship data missing in JWT token and req.user #1361

dsod opened this issue Nov 10, 2022 · 2 comments
Labels
[possible-bug] Possible bug which hasn't been reproduced yet

Comments

@dsod
Copy link
Contributor

dsod commented Nov 10, 2022

Bug Report

Current Behavior

The JWT token created in the default login operation does not contain nested relationship data. This is usually okay, due to the passport JwtStrategy middleware fetching the user from DB (with nested data) on every request. The issue is static assets, where we still perform the DB query for the user, but the nested data is not populated.

The two issues I see with this are:

  1. The read access control lacks nested relational data when run for static asset requests
  2. Inefficient (redundant DB calls)

Expected Behavior

The data provided through req.user should contain the same data for document and static asset requests. Furthermore, I think it would be a good idea to retrieve the user state from the JWT token, instead of the DB.

Possible Solution

If the JWT token is populated correctly, we could use it's state in the JwtStrategy to populate req.user, which would solve the issue with static assets. Although, I see a few problems with the current login operation:

In order to populate the JWT token properly, the afterRead hook (which effectively is the function that populates the nested data into the user object) should be moved up before the assignment of the token. The req.user assignment also needs to be moved up then, so that the afterRead passes the user state to access control functions.

When the JWT contains all the data that a fresh DB query would return, we should be able to simply re-use it's state. The only downside I can see when doing this would be that a new JWT token would have to be generated (i.e by logging out and in again) in order for the state to be updated. So, changes in the JWT state would not be reflected immediately.

Steps to Reproduce

See tests in the PR: 1362

Detailed Description

Payload version: 1.1.21

@dsod dsod added the [possible-bug] Possible bug which hasn't been reproduced yet label Nov 10, 2022
@jmikrut
Copy link
Member

jmikrut commented Nov 12, 2022

Hey @dsod I know we've got a conversation going on Discord regarding populating req.user straight from the JWT. I am not opposed to this, and I think we should keep that conversation going. But for now, I've fixed this original issue and we can keep the conversation going either on Discord or on the PR that you've opened!

@dsod
Copy link
Contributor Author

dsod commented Dec 5, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[possible-bug] Possible bug which hasn't been reproduced yet
Projects
None yet
Development

No branches or pull requests

2 participants