feat(clerk-js,localizations,shared,types): Prompt user to reset pwned… #3034

yourtallness · 2024-03-22T13:08:50Z

… password at sign-in

Description

If admin has enabled password_settings.enforce_on_sign_in and HIBP is enabled, then a password could potentially be detected as pwned at a subsequent sign-in.

The API will respond with error code form_password_pwned, in which case we will show a corresponding error and show the alternative method list, prompting them to reset their password.

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

changeset-bot · 2024-03-22T13:08:53Z

🦋 Changeset detected

Latest commit: a95a202

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages

Name	Type
@clerk/localizations	Minor
@clerk/clerk-js	Minor
@clerk/shared	Minor
@clerk/types	Minor
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/backend	Patch
@clerk/fastify	Patch
@clerk/nextjs	Patch
@clerk/clerk-react	Patch
@clerk/remix	Patch
@clerk/clerk-sdk-node	Patch
gatsby-plugin-clerk	Patch
@clerk/themes	Patch

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

yourtallness · 2024-03-22T13:10:03Z

packages/clerk-js/src/ui/components/SignIn/SignInFactorOne.tsx

@@ -135,6 +138,10 @@ export function _SignInFactorOne(): JSX.Element {
          }}
          onForgotPasswordMethodClick={resetPasswordFactor ? toggleForgotPasswordStrategies : toggleAllStrategies}
          onShowAlternativeMethodsClick={toggleAllStrategies}
+          onPasswordPwned={() => {
+            setIsPasswordPwned(true);
+            toggleForgotPasswordStrategies();


We could potentially also trigger the reset flow to start, but preferred to allow them to click it themselves.

yourtallness · 2024-03-22T13:11:32Z

packages/clerk-js/src/ui/components/SignIn/SignInFactorOnePasswordCard.tsx

@@ -40,12 +41,13 @@ const usePasswordControl = (props: SignInFactorOnePasswordProps) => {
        : onShowAlternativeMethodsClick
        ? onShowAlternativeMethodsClick
        : () => null,
+      // onPasswordPwned,


Assumed I would need to include it here as were, but this causes the following error:

Warning: Unknown event handler property onPasswordPwned. It will be ignored.

not sure why you need it here?

Me neither :-D

@desiprisg do we not need to export is like the other handlers?

Seems like we don't need it, can we remove the comment ?

yourtallness · 2024-03-22T13:11:48Z

packages/clerk-js/src/ui/components/SignIn/SignInFactorOnePasswordCard.tsx

@@ -58,6 +60,7 @@ export const SignInFactorOnePasswordCard = (props: SignInFactorOnePasswordProps)
  const clerk = useClerk();

  const goBack = () => {
+    card.setError(undefined);


This doesn't work, the error is not cleared.

Never mind, this was not the correct location to do it.

yourtallness · 2024-03-22T13:12:20Z

packages/clerk-js/src/ui/components/SignIn/SignInFactorOnePasswordCard.tsx

@@ -83,6 +86,14 @@ export const SignInFactorOnePasswordCard = (props: SignInFactorOnePasswordProps)
          return clerk.__internal_navigateWithError('..', err.errors[0]);
        }

+        if (isPasswordPwnedError(err)) {
+          if (onPasswordPwned) {
+            card.setError({ ...err.errors[0], code: err.errors[0].code + '__sign_in' });


Need to add a suffix to target the l10n key we want.

I'd say just hardcode it here since the type will always be the same.

desiprisg · 2024-03-22T13:31:46Z

packages/clerk-js/src/ui/components/SignIn/SignInFactorOnePasswordCard.tsx

@@ -83,6 +86,14 @@ export const SignInFactorOnePasswordCard = (props: SignInFactorOnePasswordProps)
          return clerk.__internal_navigateWithError('..', err.errors[0]);
        }

+        if (isPasswordPwnedError(err)) {
+          if (onPasswordPwned) {
+            card.setError({ ...err.errors[0], code: err.errors[0].code + '__sign_in' });


I'd say just hardcode it here since the type will always be the same.

desiprisg · 2024-03-22T13:32:03Z

packages/clerk-js/src/ui/components/SignIn/SignInFactorOnePasswordCard.tsx

@@ -58,6 +60,7 @@ export const SignInFactorOnePasswordCard = (props: SignInFactorOnePasswordProps)
  const clerk = useClerk();

  const goBack = () => {
+    card.setError(undefined);


Maybe we ignore undefined ? Or maybe there's a function to clear the error? Could you check the implementation?

It worked when I moved it to the proper back handlers.

desiprisg · 2024-03-22T13:33:19Z

packages/clerk-js/src/ui/components/SignIn/SignInFactorOnePasswordCard.tsx

@@ -40,12 +41,13 @@ const usePasswordControl = (props: SignInFactorOnePasswordProps) => {
        : onShowAlternativeMethodsClick
        ? onShowAlternativeMethodsClick
        : () => null,
+      // onPasswordPwned,


not sure why you need it here?

desiprisg · 2024-03-22T13:33:30Z

packages/clerk-js/src/ui/components/SignIn/SignInFactorOne.tsx

@@ -135,6 +138,10 @@ export function _SignInFactorOne(): JSX.Element {
          }}
          onForgotPasswordMethodClick={resetPasswordFactor ? toggleForgotPasswordStrategies : toggleAllStrategies}
          onShowAlternativeMethodsClick={toggleAllStrategies}
+          onPasswordPwned={() => {
+            setIsPasswordPwned(true);
+            toggleForgotPasswordStrategies();


desiprisg

If we could also get a couple of tests in to make sure this functionality doesn't break in the future, that would be superb!

Edit: Was looking at a commit. Re-reviewing..

yourtallness · 2024-03-29T10:08:54Z

If we could also get a couple of tests in to make sure this functionality doesn't break in the future, that would be superb!

I have added 2 tests (phone + pwned, email + pwned), can you recommend more scenarios?

panteliselef · 2024-03-29T10:09:21Z

packages/clerk-js/src/ui/components/SignIn/SignInFactorOnePasswordCard.tsx

@@ -40,12 +41,13 @@ const usePasswordControl = (props: SignInFactorOnePasswordProps) => {
        : onShowAlternativeMethodsClick
        ? onShowAlternativeMethodsClick
        : () => null,
+      // onPasswordPwned,


Seems like we don't need it, can we remove the comment ?

panteliselef · 2024-03-29T10:12:14Z

packages/shared/src/error.ts

@@ -67,6 +67,10 @@ export function isUserLockedError(err: any) {
  return isClerkAPIResponseError(err) && err.errors?.[0]?.code === 'user_locked';
 }

+export function isPasswordPwnedError(err: any) {
+  return isClerkAPIResponseError(err) && err.errors?.[0]?.code === 'form_password_pwned';


How come we use the first error item ? Will this always be the case from FAPI ? I see that we are doing the same above, just asking tho.

… password at sign-in

yourtallness marked this pull request as draft March 22, 2024 13:08

clerk-cookie added types clerk-js localizations labels Mar 22, 2024

yourtallness commented Mar 22, 2024

View reviewed changes

yourtallness force-pushed the yourtallness/hibp_on_sign_in branch from 7b91a29 to 8a2a243 Compare March 22, 2024 13:14

desiprisg reviewed Mar 22, 2024

View reviewed changes

yourtallness requested a review from panteliselef March 22, 2024 13:36

yourtallness force-pushed the yourtallness/hibp_on_sign_in branch 2 times, most recently from e98fd3d to 2503592 Compare March 27, 2024 17:17

yourtallness marked this pull request as ready for review March 27, 2024 17:17

yourtallness force-pushed the yourtallness/hibp_on_sign_in branch 2 times, most recently from 2982469 to f5198a2 Compare March 28, 2024 10:43

desiprisg reviewed Mar 29, 2024

View reviewed changes

panteliselef approved these changes Mar 29, 2024

View reviewed changes

feat(clerk-js,localizations,shared,types): Prompt user to reset pwned…

a95a202

… password at sign-in

yourtallness force-pushed the yourtallness/hibp_on_sign_in branch from 0060707 to a95a202 Compare March 29, 2024 10:50

yourtallness merged commit fc3ffd8 into main Mar 29, 2024
10 checks passed

yourtallness deleted the yourtallness/hibp_on_sign_in branch March 29, 2024 11:01

clerk-cookie mentioned this pull request Mar 29, 2024

Version Packages (beta) #3064

Merged

yourtallness mentioned this pull request Apr 1, 2024

feat(clerk-js,localizations,shared,types): Prompt user to reset pwned… #3075

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clerk-js,localizations,shared,types): Prompt user to reset pwned… #3034

feat(clerk-js,localizations,shared,types): Prompt user to reset pwned… #3034

yourtallness commented Mar 22, 2024

changeset-bot bot commented Mar 22, 2024 •

edited

Loading

yourtallness Mar 22, 2024

desiprisg Mar 22, 2024

yourtallness Mar 22, 2024

desiprisg Mar 22, 2024

yourtallness Mar 22, 2024

yourtallness Mar 27, 2024

panteliselef Mar 29, 2024

yourtallness Mar 22, 2024

yourtallness Mar 28, 2024

yourtallness Mar 22, 2024

desiprisg Mar 22, 2024

yourtallness Mar 22, 2024

desiprisg Mar 22, 2024

desiprisg Mar 22, 2024

yourtallness Mar 28, 2024

desiprisg Mar 22, 2024

desiprisg Mar 22, 2024

desiprisg left a comment •

edited

Loading

yourtallness commented Mar 29, 2024

panteliselef Mar 29, 2024

panteliselef Mar 29, 2024

feat(clerk-js,localizations,shared,types): Prompt user to reset pwned… #3034

feat(clerk-js,localizations,shared,types): Prompt user to reset pwned… #3034

Conversation

yourtallness commented Mar 22, 2024

Description

Checklist

Type of change

changeset-bot bot commented Mar 22, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desiprisg left a comment • edited Loading

Choose a reason for hiding this comment

yourtallness commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Mar 22, 2024 •

edited

Loading

desiprisg left a comment •

edited

Loading