-
Notifications
You must be signed in to change notification settings - Fork 79
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: password reset flows should allow some cases with no primary user and ep user existing #941
Conversation
…r and ep user existing
Deploying supertokens-node-pr-check-for-edge-function-compat with Cloudflare Pages
|
|
||
// If there is no existing primary user and there is a single option to link | ||
// we see if that user can become primary (and a candidate for linking) | ||
if (linkingCandidate === undefined && users.length === 1) { |
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.
we should also allow linking in case there is more than 1 user. Same way as the other function which picks the oldest user and tries to link with that. Ideallyt we just reuse that function
// If a primary user has the input email as verified or has no other emails: then it is always allowed to reset their password: | ||
// - there is no risk of account takeover, because they have verified this email or haven't linked it to anything else | ||
// - there will be no linking as a result of this action, so we do not need to check for linking. | ||
// TODO: what happens if user M signs up with the email of the victim using a third-party provider that doesn't verify emails |
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.
// TODO: what happens if user M signs up with the email of the victim using a third-party provider that doesn't verify emails |
// extra security measure here to make sure that the input email in the primary user | ||
// is verified, and if not, we need to make sure that there is no other email / phone number | ||
// associated with the primary user account. If there is, then we do not proceed. | ||
|
||
/* | ||
This security measure helps prevent the following attack: |
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.
move this comment above as well
); | ||
} | ||
|
||
if (!existingUser.isPrimaryUser) { |
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.
remove this if block
Summary of change
(A few sentences about this PR)
Related issues
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)
Documentation changes
(If relevant, please create a PR in our docs repo, or create a checklist here highlighting the necessary changes)
Checklist for important updates
coreDriverInterfaceSupported.json
file has been updated (if needed)lib/ts/version.ts
frontendDriverInterfaceSupported.json
file has been updated (if needed)package.json
package-lock.json
lib/ts/version.ts
npm run build-pretty
recipe/thirdparty/providers/configUtils.ts
file,createProvider
function.git tag
) in the formatvX.Y.Z
, and then find the latest branch (git branch --all
) whoseX.Y
is greater than the latest released tag.add-ts-no-check.js
file to include thatsomeFunc: function () {..}
).exports
inpackage.json
Remaining TODOs for this PR