-
Notifications
You must be signed in to change notification settings - Fork 554
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
Support captchas in reset password flow #2547
Support captchas in reset password flow #2547
Conversation
Will bump the auth0.js version after this PR is merged. |
f84445c
to
20a8ecb
Compare
Current build failures will also be fixed once auth0.js version is bumped |
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.
LGTM, just had a question about some parts of the code API.
src/connection/captcha.js
Outdated
export function getCaptchaConfig(m, isPasswordless, isPasswordReset) { | ||
if (isPasswordReset) { | ||
return l.resetPasswordCaptcha(m); | ||
} else if (isPasswordless) { | ||
return l.passwordlessCaptcha(m); | ||
} else { | ||
return l.captcha(m); | ||
} | ||
} |
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.
I'm not sure this is a great API for this function. The two input booleans are actually mutually exclusive when looking at the implementation but someone calling this method would reasonably expect that they can provide any combination of true
and false
here and each combination would have its own result.
I can see this pattern used throughout the PR - why was this decision taken vs using something else, maybe a number value and some constants?
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.
I just tried to follow the existing pattern with the isPasswordless boolean but you're right it could be confusing to the caller. I'll try to change it to a single value.
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.
updated
@stevehobbsdev Any ideas why the build is failing on install dependencies? Seems to be the case after bumping auth0-js? Maybe I don't have permissions to bump packages? |
@srijonsaha Yeah you'll need to make sure it's pulling the library from the NPM registry, as our customers won't be able to connect to our internal one. https://github.com/srijonsaha/lock/blob/reset-password-challenge/package-lock.json#L5051 |
**Added** - Support captchas in reset password flow [\#2547](#2547) ([srijonsaha](https://github.com/srijonsaha))
Changes
Adds support for captchas in the reset password flow for classic login.
References
https://auth0team.atlassian.net/browse/IAMRISK-3340
Testing
test.mov
Checklist