Skip to content

feat(clerk-js, types): Trigger UserVerification within UserProfile#4127

Merged
panteliselef merged 32 commits intomainfrom
elef/user-665-display-userverification-for-sensitive-actions-in
Oct 1, 2024
Merged

feat(clerk-js, types): Trigger UserVerification within UserProfile#4127
panteliselef merged 32 commits intomainfrom
elef/user-665-display-userverification-for-sensitive-actions-in

Conversation

@panteliselef
Copy link
Copy Markdown
Contributor

@panteliselef panteliselef commented Sep 9, 2024

Description

When user re-verification is required FAPI will return a specific error. The goal of this PR is to recognise that error and display the UserVerification modal. After the verification is successful we retry the original request, but if the verification is cancelled the original request will not be retried.

How are people not affected by this

This is a pretty core change in the UserProfile component, we need to make sure that only application that have opted-in to the experimental flows will see the new behavior of UserProfile. This is ensured by many existing tests including this one that would hang indefinitely based on the new behavior.

Cancelled verification

Screen.Recording.2024-09-16.at.6.01.43.PM.mov

Successful verification

Screen.Recording.2024-09-16.at.6.06.35.PM.mov

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

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselef panteliselef self-assigned this Sep 9, 2024
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Sep 9, 2024

🦋 Changeset detected

Latest commit: f7dad10

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

This PR includes changesets to release 18 packages
Name Type
@clerk/clerk-js Minor
@clerk/types Minor
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
@clerk/astro Patch
@clerk/backend Patch
@clerk/elements Patch
@clerk/express Patch
@clerk/fastify Patch
@clerk/localizations Patch
@clerk/nextjs Patch
@clerk/clerk-react Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch
@clerk/shared Patch
@clerk/tanstack-start Patch
@clerk/testing 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

{ wrapper },
);
await userEvent.click(getByRole('button', { name: 'Add a passkey' }));
getByRole('button', { name: 'Add a passkey' });
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🧹 We were not doing anything after the click.

Comment thread packages/clerk-js/src/ui/foundations/zIndices.ts Outdated
Comment thread packages/clerk-js/src/ui/elements/Menu.tsx Outdated
@panteliselef panteliselef changed the title Elef/user 665 display userverification for sensitive actions in feat: Trigger UserVerification within UserProfile Sep 16, 2024
@panteliselef panteliselef changed the title feat: Trigger UserVerification within UserProfile feat(clerk-js): Trigger UserVerification within UserProfile Sep 16, 2024
@panteliselef panteliselef changed the title feat(clerk-js): Trigger UserVerification within UserProfile feat(clerk-js, types): Trigger UserVerification within UserProfile Sep 16, 2024
@panteliselef panteliselef marked this pull request as ready for review September 16, 2024 15:47
@panteliselef
Copy link
Copy Markdown
Contributor Author

!preview

1 similar comment
@panteliselef
Copy link
Copy Markdown
Contributor Author

!preview

Comment on lines +87 to +89
options?: {
notify?: boolean;
},
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is only internal

Comment thread .changeset/four-oranges-clap.md Outdated
Comment thread packages/clerk-js/src/ui/Components.tsx Outdated
Comment on lines +29 to +31
metadata,
},
} as AssuranceHint;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not add the metadata to the AssuranceHint, then you don't need to typecast

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I may end up changing this in a followup iteration.

Comment thread packages/clerk-js/src/ui/hooks/useAssurance.ts Outdated
@alexcarpenter alexcarpenter requested a review from a team September 27, 2024 13:37
@panteliselef
Copy link
Copy Markdown
Contributor Author

!snapshot

@clerk-cookie
Copy link
Copy Markdown
Collaborator

Hey @panteliselef - the snapshot version command generated the following package versions:

Package Version
@clerk/astro 1.3.9-snapshot.vbe1abf5
@clerk/backend 1.13.5-snapshot.vbe1abf5
@clerk/chrome-extension 1.3.11-snapshot.vbe1abf5
@clerk/clerk-js 5.24.0-snapshot.vbe1abf5
@clerk/elements 0.15.7-snapshot.vbe1abf5
@clerk/clerk-expo 2.2.17-snapshot.vbe1abf5
@clerk/express 1.0.2-snapshot.vbe1abf5
@clerk/fastify 1.0.48-snapshot.vbe1abf5
@clerk/localizations 3.1.0-snapshot.vbe1abf5
@clerk/nextjs 5.6.3-snapshot.vbe1abf5
@clerk/clerk-react 5.9.4-snapshot.vbe1abf5
@clerk/remix 4.2.32-snapshot.vbe1abf5
@clerk/clerk-sdk-node 5.0.45-snapshot.vbe1abf5
@clerk/shared 2.8.4-snapshot.vbe1abf5
@clerk/tanstack-start 0.4.8-snapshot.vbe1abf5
@clerk/testing 1.3.6-snapshot.vbe1abf5
@clerk/themes 2.1.33-snapshot.vbe1abf5
@clerk/types 4.23.0-snapshot.vbe1abf5

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/astro

npm i @clerk/astro@1.3.9-snapshot.vbe1abf5 --save-exact

@clerk/backend

npm i @clerk/backend@1.13.5-snapshot.vbe1abf5 --save-exact

@clerk/chrome-extension

npm i @clerk/chrome-extension@1.3.11-snapshot.vbe1abf5 --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@5.24.0-snapshot.vbe1abf5 --save-exact

@clerk/elements

npm i @clerk/elements@0.15.7-snapshot.vbe1abf5 --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@2.2.17-snapshot.vbe1abf5 --save-exact

@clerk/express

npm i @clerk/express@1.0.2-snapshot.vbe1abf5 --save-exact

@clerk/fastify

npm i @clerk/fastify@1.0.48-snapshot.vbe1abf5 --save-exact

@clerk/localizations

npm i @clerk/localizations@3.1.0-snapshot.vbe1abf5 --save-exact

@clerk/nextjs

npm i @clerk/nextjs@5.6.3-snapshot.vbe1abf5 --save-exact

@clerk/clerk-react

npm i @clerk/clerk-react@5.9.4-snapshot.vbe1abf5 --save-exact

@clerk/remix

npm i @clerk/remix@4.2.32-snapshot.vbe1abf5 --save-exact

@clerk/clerk-sdk-node

npm i @clerk/clerk-sdk-node@5.0.45-snapshot.vbe1abf5 --save-exact

@clerk/shared

npm i @clerk/shared@2.8.4-snapshot.vbe1abf5 --save-exact

@clerk/tanstack-start

npm i @clerk/tanstack-start@0.4.8-snapshot.vbe1abf5 --save-exact

@clerk/testing

npm i @clerk/testing@1.3.6-snapshot.vbe1abf5 --save-exact

@clerk/themes

npm i @clerk/themes@2.1.33-snapshot.vbe1abf5 --save-exact

@clerk/types

npm i @clerk/types@4.23.0-snapshot.vbe1abf5 --save-exact

Copy link
Copy Markdown
Member

@alexcarpenter alexcarpenter left a comment

Choose a reason for hiding this comment

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

Hitting the back button removes the dialog but not the overlay.

Linked video issue in slack.

Would be nice if there was only one overlay instead of the stacking effect where it gets darker. non-blocker though.

@alexcarpenter alexcarpenter dismissed their stale review September 27, 2024 14:54

Shared in slack

{showPhone && <PhoneSection shouldAllowCreation={shouldAllowIdentificationCreation} />}
{showConnectedAccounts && <ConnectedAccountsSection shouldAllowCreation={shouldAllowIdentificationCreation} />}

{/*TODO-STEP-UP: Verify that these work as expected*/}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these verified they work? 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, although thorough testing will still need to be done. E2E tests will also be introduced before this goes to public beta

return session.revoke().finally(() => status.setIdle());
return (
handleAssurance(() => session.revoke())
// TODO-STEPUP: Properly handler the response with a setCardError
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@panteliselef is this to-do for later or it's forgotten?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually this is not step-up specific, just something that i discovered (for later)

if (afterVerificationUrl) {
await navigate(afterVerificationUrl);
}
throw 'afterVerification is only triggered in modals';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should never happen, right? We don't expect it to ever happen (?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, a follow up PR will remove UserVerification as mounted component

Comment on lines +12 to +31
| Operation | Reverification | Strategy | Timeframe |
| --- |----------------| --- | --- |
| Update account (first/last name) | ❌ | | |
| Update username | ✅ | Strongest available | 10m |
| Delete account | ✅ | Strongest available | 10m |
| Create/Remove profile image | ❌ | | |
| Update password | ✅ | Strongest available | 10m |
| Remove password | ❌ | | |
| Revoke session | ✅ | Strongest available | 10m |
| Create identification | ✅ | Strongest available | 10m |
| Remove identification | ✅ | Strongest available | 10m |
| Change primary identification | ✅ | Strongest available | 10m |
| Update Passkey name | ❌ | | |
| Enable MFA (TOTP, Phone number) | ✅ | Strongest available | 10m |
| Disable MFA (TOΤP, Phone number) | ✅ | Strongest available | 10m |
| Create/Regenerate Backup Codes | ✅ | Strongest available | 10m |
| Connect External Account | ✅ | Strongest available | 10m |
| Re-authorize External Account | ❌ | | |
| Remove External Account | ✅ | Strongest available | 10m |
| Leave organization | ❌ | | |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this table also going into our docs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Eventually, yes we currently have it in our DX gudie

Copy link
Copy Markdown
Member

@octoper octoper left a comment

Choose a reason for hiding this comment

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

Except from the comments Stefanos shared above this looks good 🔥

Copy link
Copy Markdown
Member

@anagstef anagstef left a comment

Choose a reason for hiding this comment

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

Great work! 💯

@panteliselef panteliselef merged commit 4a85705 into main Oct 1, 2024
@panteliselef panteliselef deleted the elef/user-665-display-userverification-for-sensitive-actions-in branch October 1, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants