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
@keystone-6/auth
assumes a non-zero itemId
#8843
Comments
@mmtftr nice find! |
As you mentioned, we have a few ways we could approach this, when 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 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. |
@mmtftr alternatively, we could prevent an identifier of |
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: keystone/packages/auth/src/index.ts Lines 172 to 174 in 60357b7
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 |
I don't think we want to add warnings by default for something like this, but you can always wrap the session strategy yourself. |
@keystone-6/auth
assumes a non-zero itemId
keystone/packages/auth/src/index.ts
Line 173 in 60357b7
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)
The text was updated successfully, but these errors were encountered: