-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix bug in passwordResetLink #4965
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/keystonejs/keystone-next-docs/6QjhrPC6YGzjCvFtGyEf5X63poRM |
🦋 Changeset detectedLatest commit: 54112a6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
You mean |
So overall I think I'm in favour of this, but I'd like to go more comprehensively in one direction or the other. These fields were supposed to be variable, because it's a "secret" and may not be called "password". But can we be consistent? it's configured as a I don't think I'm unhappy with this PR, since among other things it fixes a bug, but I do think I'm unhappy with the overall balance of apparently-configurable vs not-actually-configurable combined with (I think) we use two words for the same thing. @timleslie if you agree, we should probably backlog another pass to clean this up properly even further, align on one consistent term, and be more opinionated overall in the name of simplicity (tbh at this point I'd go for "password" over "secret", and I think "secret" is a hangover from Keystone 5 when it genuinely was less opinionated) (to be clear, I'm not suggesting we hard code the two primary fields - currently, |
.changeset/soft-stingrays-happen.md
Outdated
@@ -0,0 +1,5 @@ | |||
--- | |||
'@keystone-next/auth': patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a patch; it changes database structure for anyone using auth
with a field path other than password
for the secret field.
We can decide to ship this as a breaking change, or we can fix the bug in the other direction (by making getPasswordResetSchema.ts
generate the field path dynamically based on the secretField
config when the tokenType
is passwordReset
)
d36c0f3
to
44dcddb
Compare
44dcddb
to
b410d79
Compare
0de0a0c
to
11652e3
Compare
11652e3
to
66070f1
Compare
These field names need to correspond to
const tokenType = 'passwordReset';
ingetPasswordSchemaReset.ts
. They aren't meant to be dynamic on the field name.