`@keystone-6/auth` assumes a non-zero `itemId`

[mmtftr]

https://github.com/keystonejs/keystone/blob/60357b76c6eac1c3727b8cb77059e8d7908b92aa/packages/auth/src/index.ts#L173

I had a user with ID===0 where the field was autoincrement. Can we support this use case? This check should either be "itemId" in session or !session.itemId && session.itemId !== 0)

[dcousens]

@mmtftr nice find! Did you want to open a pull request? :blue_heart:

[dcousens]

As you mentioned, we have a few ways we could approach this, when session.itemid could be null, undefined or missing.

if (!('itemId' in session))
/* 
  Results
    0: true
    '': true
    undefined: true
    null: true
    missing: false
*/

I don't think we can recommend this approach, users might mistakenly returning itemId: undefined | null from session.get, which could leave them with an invalid session. That and empty string is now accepted, which it shouldn't be.

---

if (typeof session.itemId !== 'string' && typeof session.itemId !== 'number')
/* 
  Results
    0: true
    '': true
    undefined: false
    null: false
    missing: false
*/

This approach is nearly there, except empty string is now accepted.

---

if (!session.itemId && session.itemId !== 0)
/* 
  Results
    0: true
    '': false
    undefined: false
    null: false
    missing: false
*/

Not bad, but it leaves the reader questioning what we're actually trying to do.

---

if (
  session.itemId === undefined 
  || sesion.itemId === null
  || sesion.itemId === ''
)
/* 
  Results
    0: true
    '': false
    undefined: false
    null: false
    missing: false
*/

This may be the most verbose, but, it's clear what we're trying to do?

Interested to see how this plays out in the pull request, and if we have any tests to this effect.

[dcousens]

@mmtftr alternatively, we could prevent an identifier of 0 in a way that is a little more consistent

[mmtftr]

I think another important point is that this behavior is not at all documented and these problems with the session object are not reported to the administrator/developer. I had to read through the Keystone source code to find out why I didn't have sessions despite logging in. I am also not familiar with how to debug a Next.js application in dev mode (Using a JavaScript Debug Terminal didn't work, for some reason I couldn't find any Keystone code that was loaded, there was some loaded WASM code).

Thus, I think adding a console warning log or some other kind of warning is appropriate for the latter 2 if statements in the following code snippet:

https://github.com/keystonejs/keystone/blob/60357b76c6eac1c3727b8cb77059e8d7908b92aa/packages/auth/src/index.ts#L172-L174

The first return is an expected case where we don't have a session but the other two cases are when a sessionStrategy returns an unexpected session object that's not compatible with keystone auth. I believe warning the user is the right choice here, I'm not specific about how that warning happens though.

For the actual check, I could write it concisely like [undefined, null, ''].includes(session.itemId) which satisfies the expected behavior.

[dcousens]

I don't think we want to add warnings by default for something like this, but you can always wrap the session strategy yourself. @mmtftr did you want to open a pull request?