magda icon indicating copy to clipboard operation
magda copied to clipboard

Tidy up `req.user`'s Structure & Life Cycle

Open t83714 opened this issue 5 years ago • 0 comments

Is your feature request related to a problem? Please describe.

We might want to tidy up code around req.user among all modules for some inconsistencies.

Mainly because req.user at this moment can:

  • have a different value
  • different structure)
  • and sometimes not exist / not available at all

And the actual status of req.user at this moment depends on many things, they are:

  • request processing life cycle
  • request routes
  • middleware / lib get involved (particularly this one)

e.g. req.user's value might be set under 3 scenarios:

  • recover a session from cookie
    • In this case, req.user 's value is { id: "xxxx" }. Internally, we call it UserToken type
  • authenticate through API Key
    • Same as above
  • micro-services that receives request with JWT
    • the micro-service didn't use mustBeAdmin middleware (e.g. only used mustBeLoggedIn middleware)
      • req.user will be undefined
    • the micro-service used mustBeAdmin middleware and the user is an admin
      • In this case, req.user 's value would be an object that contains all user properties (e.g. displayName, isAdmin etc.)

Those inconsistencies cause confusion and might lead to errors.

Describe the solution you'd like

To reduce inconsistencies, I think we can make sure:

  • if req.user exists, it must include all properties of user (i.e. all columns of user table), rather than just id field instead.
    • This requires all oAuth plugin should call passport callback with complete user object (instead of id only)
    • JWT token should carry the user object with all properties of user (so that when we need to recover req.user` from JWT, we don't have to fetch it from the database again)
  • Stop decoding JWT in middleware mustBeAdmin or mustBeLoggedIn and move JWT decoding (plus req.user recovery function ) to a common middleware useJwtToken.
    • mustBeAdmin or mustBeLoggedIn should complete their logic purely based on req.user's data

t83714 avatar Jul 16 '20 07:07 t83714