Skip to content
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

IAMRISK-1790 Support captcha for Passwordless #2222

Merged

Conversation

robinbijlani
Copy link
Contributor

@robinbijlani robinbijlani commented Dec 14, 2022

Changes

This adds support for captcha in Passwordless flows (Classic Universal Login).

For context, to allow this, the Authentication API has some changes:

  • POST /passwordless/challenge can be hit to know if a captcha should be shown for Passwordless flows.
  • POST /passwordless/start will now verify the captcha field if one is required.

Here are a few demo recordings of the behavior:

passwordless-email-auth0

passwordless-email-recap

passwordless-sms-auth0
☝️ This recording shows the one clunky UX issue I found. When using restart() to go back to the first pane, I can't figure out how to clear out the captcha field from the model. (I tried several implementations of ('../../field/index').clearFields(m, ['captcha']); combined with swap() but I am think I'm somewhat foggy on how the model binding works here.)

passwordless-sms-recap

References

https://auth0team.atlassian.net/browse/IAMRISK-1790

Testing

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of the platform/language

Checklist

kaki1104 and others added 14 commits December 14, 2022 15:44
Fixes for a couple of issues:

**Circular dependency error**

Thanks to the email field, it was trying to use a function `isHRDEmailValid` from 'connection/enterprise', which isn't necessary for this test (I don't *think* you can have enterprise passwordless connections, you would just use the non-passwordless version of Lock). Mocking out this module and just returning `false` for `isHRDEmailValid` makes things simpler.

**m.getIn is not a function**

This is down to `social_or_email_login_screen` calling `hasSomeConnections` from 'core/index', this can simply be mocked to return `true` for this test. This function just verifies that there is a passwordless or email connection available.

I also had to mock out i18n.html, as this function is called when the component renders.
@robinbijlani robinbijlani marked this pull request as ready for review December 14, 2022 21:42
@robinbijlani robinbijlani requested a review from a team as a code owner December 14, 2022 21:42
@@ -28,15 +28,25 @@ function getErrorMessage(m, error) {
key = 'bad.phone_number';
}

if (error.code === 'invalid_captcha') {
const captchaConfig = l.passwordlessCaptcha(m);
key = captchaConfig.get('provider') === 'recaptcha_v2' ? 'invalid_recaptcha' : 'invalid_captcha';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to think about recaptcha enterprise here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the callout. Since recaptcha_v2 and recaptcha_enterprise both offer the same UX, I've made the error messages match everywhere we are showing captcha. 👍

src/connection/passwordless/actions.js Show resolved Hide resolved
@@ -430,6 +436,13 @@ export function captcha(m) {
return get(m, 'captcha');
}

export function passwordlessCaptcha(m) {
if (typeof m !== 'object') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check necessary? Should get handle non-objects (just thinking of lodash)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied it from the captcha() function above, but it looks like this isn't needed for the current tests. 🤔 For now I removed it from both, but maybe @stevehobbsdev can double check this.

src/i18n/af.js Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@stevehobbsdev
Copy link
Contributor

stevehobbsdev commented Dec 15, 2022

I can't figure out how to clear out the captcha field from the model. (I tried several implementations of ('../../field/index').clearFields(m, ['captcha']); combined with swap() but I am think I'm somewhat foggy on how the model binding works here.)

@robinbijlani there is a reset helper on the captcha field that should allow you to reset it, did you try this one?

This is used on Captcha for non-passwordless when you click on the 'change captcha' button to get a new code, so I'm wondering if it can be re-used for your scenario when you hit the back button.

import * as captchaField from '../field/captcha';

// ....

export function setCaptcha(m, value, wasInvalid) {
  m = captchaField.reset(m, wasInvalid);
  return set(m, 'captcha', Immutable.fromJS(value));
}

I noticed in your first video that when you click on that button the field doesn't reset, whereas today it does (for non-Passwordless captcha), so wondering if there's some issue with passwordlessCaptchaField.reset (but it looks ok when compared to captchaField.reset) 🤔

@robinbijlani
Copy link
Contributor Author

robinbijlani commented Dec 15, 2022

@stevehobbsdev Thank you for the guidance! I realized my mistake was that while we have a passwordlessCaptcha prop on the model (to store info about the captcha challenge we display), we have no separate field to enter the passwordless captcha, and are reusing the existing captcha one. Now things seem to be behaving like I would expect:

passwordless-email-auth0-v2
passwordless-email-recap-v2

@robinbijlani
Copy link
Contributor Author

Also, the Browserstack failure is expected, right? 🤔

No browserstack username!
No browserstack password!

@robinbijlani
Copy link
Contributor Author

robinbijlani commented Dec 16, 2022

@stevehobbsdev Also, I know you're OOF until Monday, but when you get back, if this looks good I'd love for a new release of Lock to be cut. (I want to bump the default version used from auth0-server if it ends up being higher than 11.34.x, which is currently used.)

@stevehobbsdev stevehobbsdev self-requested a review December 19, 2022 15:33
@stevehobbsdev
Copy link
Contributor

Also, the Browserstack failure is expected, right?

@robinbijlani Yes, as these changes come from a fork, the Browserstack run does not get the secrets required to execute. I will check the E2E tests locally.

@stevehobbsdev stevehobbsdev merged commit f8c87c0 into auth0:master Dec 19, 2022
@stevehobbsdev stevehobbsdev mentioned this pull request Dec 19, 2022
stevehobbsdev added a commit that referenced this pull request Jan 20, 2023
* Bump auth0-js from 9.19.2 to 9.20.0 (#2221)

Bumps [auth0-js](https://github.com/auth0/auth0.js) from 9.19.2 to 9.20.0.
- [Release notes](https://github.com/auth0/auth0.js/releases)
- [Changelog](https://github.com/auth0/auth0.js/blob/master/CHANGELOG.md)
- [Commits](auth0/auth0.js@v9.19.2...v9.20.0)

---
updated-dependencies:
- dependency-name: auth0-js
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* IAMRISK-1790 Support captcha for Passwordless (#2222)

* initial changes for email passwordless login screen

* updated tests

* Fix test for passwordless/social_or_email_login_screen

Fixes for a couple of issues:

**Circular dependency error**

Thanks to the email field, it was trying to use a function `isHRDEmailValid` from 'connection/enterprise', which isn't necessary for this test (I don't *think* you can have enterprise passwordless connections, you would just use the non-passwordless version of Lock). Mocking out this module and just returning `false` for `isHRDEmailValid` makes things simpler.

**m.getIn is not a function**

This is down to `social_or_email_login_screen` calling `hasSomeConnections` from 'core/index', this can simply be mocked to return `true` for this test. This function just verifies that there is a passwordless or email connection available.

I also had to mock out i18n.html, as this function is called when the component renders.

* add capthca pane to social or email login screen (failing tests)

* Remove unneeded lines

* add captcha to passwordless login screens, with unit tests passing

* got rid of sso

* got rid of enterprise check

* deleted unncessary imports

* Captcha support for Passwordless

* Update passwordless snapshots

* Error translations

* Swap captcha if restarting passwordless

* Add missing fun argument docs

* Use invalid_recaptcha error key for recaptcha_enterprise

* Resolve deps publicly

* Fix bug to correctly reset captcha field

* Remove conditional for non objects

* Add missing translations

Co-authored-by: kaki1104 <[email protected]>
Co-authored-by: Steve Hobbs <[email protected]>

* Release v11.35.0 (#2223)

release v11.35.0

* Bump eslint-config-prettier from 8.5.0 to 8.6.0

Bumps [eslint-config-prettier](https://github.com/prettier/eslint-config-prettier) from 8.5.0 to 8.6.0.
- [Release notes](https://github.com/prettier/eslint-config-prettier/releases)
- [Changelog](https://github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md)
- [Commits](prettier/eslint-config-prettier@v8.5.0...v8.6.0)

---
updated-dependencies:
- dependency-name: eslint-config-prettier
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Bump dompurify from 2.4.1 to 2.4.3 (#2232)

Bumps [dompurify](https://github.com/cure53/DOMPurify) from 2.4.1 to 2.4.3.
- [Release notes](https://github.com/cure53/DOMPurify/releases)
- [Commits](cure53/DOMPurify@2.4.1...2.4.3)

---
updated-dependencies:
- dependency-name: dompurify
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump prettier from 2.8.1 to 2.8.2 (#2231)

Bumps [prettier](https://github.com/prettier/prettier) from 2.8.1 to 2.8.2.
- [Release notes](https://github.com/prettier/prettier/releases)
- [Changelog](https://github.com/prettier/prettier/blob/main/CHANGELOG.md)
- [Commits](prettier/prettier@2.8.1...2.8.2)

---
updated-dependencies:
- dependency-name: prettier
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump eslint-plugin-react from 7.31.11 to 7.32.0 (#2233)

Bumps [eslint-plugin-react](https://github.com/jsx-eslint/eslint-plugin-react) from 7.31.11 to 7.32.0.
- [Release notes](https://github.com/jsx-eslint/eslint-plugin-react/releases)
- [Changelog](https://github.com/jsx-eslint/eslint-plugin-react/blob/master/CHANGELOG.md)
- [Commits](jsx-eslint/eslint-plugin-react@v7.31.11...v7.32.0)

---
updated-dependencies:
- dependency-name: eslint-plugin-react
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump prettier from 2.8.2 to 2.8.3 (#2237)

* Bump auth0-js from 9.20.0 to 9.20.1 (#2235)

* Bump eslint-plugin-react from 7.32.0 to 7.32.1 (#2238)

Bumps [eslint-plugin-react](https://github.com/jsx-eslint/eslint-plugin-react) from 7.32.0 to 7.32.1.
- [Release notes](https://github.com/jsx-eslint/eslint-plugin-react/releases)
- [Changelog](https://github.com/jsx-eslint/eslint-plugin-react/blob/master/CHANGELOG.md)
- [Commits](jsx-eslint/eslint-plugin-react@v7.32.0...v7.32.1)

---
updated-dependencies:
- dependency-name: eslint-plugin-react
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump node-fetch from 2.6.7 to 2.6.8 (#2236)

Bumps [node-fetch](https://github.com/node-fetch/node-fetch) from 2.6.7 to 2.6.8.
- [Release notes](https://github.com/node-fetch/node-fetch/releases)
- [Commits](node-fetch/node-fetch@v2.6.7...v2.6.8)

---
updated-dependencies:
- dependency-name: node-fetch
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Rita Zerrizuela <[email protected]>
Co-authored-by: Steve Hobbs <[email protected]>

* Use latest ship-orb in CI (#2234)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Robin Bijlani <[email protected]>
Co-authored-by: kaki1104 <[email protected]>
Co-authored-by: Steve Hobbs <[email protected]>
Co-authored-by: Rita Zerrizuela <[email protected]>
stevehobbsdev added a commit that referenced this pull request Jan 20, 2023
* Remove Bower support (#2198)

remove build assets, bower.json and exclude

* [SDK-3789] Upgrade to React 18 (#2209)

* install react, react-dom 18

* use createRoot instead of render

* use @cfaester/enzyme-adapter-react-18 and update snapshots

* fix up async issues in E2E tests

* run yarn upgrade

* fix up another failing async test with helpers

* use lts browsers in Circle build

* remove dependency on 'node-fetch'

* use UNSAFE_ for componentWilReceiveProps

* [SDK-3796] Upgrade to Webpack 5 (#2213)

* use @cfaester/enzyme-adapter-react-18 and update snapshots

* fix up async issues in E2E tests

* run yarn upgrade

* upgrade to webpack 3

* upgrade webpack to 4.x

* wip - upgrade to babel 7

* upgrade UnminifiedWebpackPlugin

* update jest to latest and update tests & snapshots

* update karma dependencies

* upgrade to webpack 5

* adding polyfills

* tweaks to fix e2e tests

* enable browserstack tests on IE11

* attempting to fix concurrency issues in browserstack runs

* add new line for browserslistrc

* Update src/__tests__/core/index.test.js

Co-authored-by: Rita Zerrizuela <[email protected]>

Co-authored-by: Rita Zerrizuela <[email protected]>

* [chore] update readme for the beta (#2217)

Update readme for the beta

* release v12.0.0-beta.0 (#2218)

* Update circle config to use latest ship-orb (#2219)

* Update README.md (#2220)

* Update readme for release (#2239)

* chore: update docs for GA release

* chore: build lockfile using yarn@2

* Merge master into beta (#2240)

* Bump auth0-js from 9.19.2 to 9.20.0 (#2221)

Bumps [auth0-js](https://github.com/auth0/auth0.js) from 9.19.2 to 9.20.0.
- [Release notes](https://github.com/auth0/auth0.js/releases)
- [Changelog](https://github.com/auth0/auth0.js/blob/master/CHANGELOG.md)
- [Commits](auth0/auth0.js@v9.19.2...v9.20.0)

---
updated-dependencies:
- dependency-name: auth0-js
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* IAMRISK-1790 Support captcha for Passwordless (#2222)

* initial changes for email passwordless login screen

* updated tests

* Fix test for passwordless/social_or_email_login_screen

Fixes for a couple of issues:

**Circular dependency error**

Thanks to the email field, it was trying to use a function `isHRDEmailValid` from 'connection/enterprise', which isn't necessary for this test (I don't *think* you can have enterprise passwordless connections, you would just use the non-passwordless version of Lock). Mocking out this module and just returning `false` for `isHRDEmailValid` makes things simpler.

**m.getIn is not a function**

This is down to `social_or_email_login_screen` calling `hasSomeConnections` from 'core/index', this can simply be mocked to return `true` for this test. This function just verifies that there is a passwordless or email connection available.

I also had to mock out i18n.html, as this function is called when the component renders.

* add capthca pane to social or email login screen (failing tests)

* Remove unneeded lines

* add captcha to passwordless login screens, with unit tests passing

* got rid of sso

* got rid of enterprise check

* deleted unncessary imports

* Captcha support for Passwordless

* Update passwordless snapshots

* Error translations

* Swap captcha if restarting passwordless

* Add missing fun argument docs

* Use invalid_recaptcha error key for recaptcha_enterprise

* Resolve deps publicly

* Fix bug to correctly reset captcha field

* Remove conditional for non objects

* Add missing translations

Co-authored-by: kaki1104 <[email protected]>
Co-authored-by: Steve Hobbs <[email protected]>

* Release v11.35.0 (#2223)

release v11.35.0

* Bump eslint-config-prettier from 8.5.0 to 8.6.0

Bumps [eslint-config-prettier](https://github.com/prettier/eslint-config-prettier) from 8.5.0 to 8.6.0.
- [Release notes](https://github.com/prettier/eslint-config-prettier/releases)
- [Changelog](https://github.com/prettier/eslint-config-prettier/blob/main/CHANGELOG.md)
- [Commits](prettier/eslint-config-prettier@v8.5.0...v8.6.0)

---
updated-dependencies:
- dependency-name: eslint-config-prettier
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Bump dompurify from 2.4.1 to 2.4.3 (#2232)

Bumps [dompurify](https://github.com/cure53/DOMPurify) from 2.4.1 to 2.4.3.
- [Release notes](https://github.com/cure53/DOMPurify/releases)
- [Commits](cure53/DOMPurify@2.4.1...2.4.3)

---
updated-dependencies:
- dependency-name: dompurify
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump prettier from 2.8.1 to 2.8.2 (#2231)

Bumps [prettier](https://github.com/prettier/prettier) from 2.8.1 to 2.8.2.
- [Release notes](https://github.com/prettier/prettier/releases)
- [Changelog](https://github.com/prettier/prettier/blob/main/CHANGELOG.md)
- [Commits](prettier/prettier@2.8.1...2.8.2)

---
updated-dependencies:
- dependency-name: prettier
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump eslint-plugin-react from 7.31.11 to 7.32.0 (#2233)

Bumps [eslint-plugin-react](https://github.com/jsx-eslint/eslint-plugin-react) from 7.31.11 to 7.32.0.
- [Release notes](https://github.com/jsx-eslint/eslint-plugin-react/releases)
- [Changelog](https://github.com/jsx-eslint/eslint-plugin-react/blob/master/CHANGELOG.md)
- [Commits](jsx-eslint/eslint-plugin-react@v7.31.11...v7.32.0)

---
updated-dependencies:
- dependency-name: eslint-plugin-react
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump prettier from 2.8.2 to 2.8.3 (#2237)

* Bump auth0-js from 9.20.0 to 9.20.1 (#2235)

* Bump eslint-plugin-react from 7.32.0 to 7.32.1 (#2238)

Bumps [eslint-plugin-react](https://github.com/jsx-eslint/eslint-plugin-react) from 7.32.0 to 7.32.1.
- [Release notes](https://github.com/jsx-eslint/eslint-plugin-react/releases)
- [Changelog](https://github.com/jsx-eslint/eslint-plugin-react/blob/master/CHANGELOG.md)
- [Commits](jsx-eslint/eslint-plugin-react@v7.32.0...v7.32.1)

---
updated-dependencies:
- dependency-name: eslint-plugin-react
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump node-fetch from 2.6.7 to 2.6.8 (#2236)

Bumps [node-fetch](https://github.com/node-fetch/node-fetch) from 2.6.7 to 2.6.8.
- [Release notes](https://github.com/node-fetch/node-fetch/releases)
- [Commits](node-fetch/node-fetch@v2.6.7...v2.6.8)

---
updated-dependencies:
- dependency-name: node-fetch
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Rita Zerrizuela <[email protected]>
Co-authored-by: Steve Hobbs <[email protected]>

* Use latest ship-orb in CI (#2234)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Robin Bijlani <[email protected]>
Co-authored-by: kaki1104 <[email protected]>
Co-authored-by: Steve Hobbs <[email protected]>
Co-authored-by: Rita Zerrizuela <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Rita Zerrizuela <[email protected]>
Co-authored-by: Frederik Prijck <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Robin Bijlani <[email protected]>
Co-authored-by: kaki1104 <[email protected]>
Co-authored-by: Steve Hobbs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants