A new password change wizard #38728

bl-nero · 2024-02-28T14:50:57Z

Closes #36233
Closes #36232
Partially implements RFD 0159.
Figma designs

What's this?

This change turns the old password change dialog into a two-step wizard. The first step is picking the identity verification method (skipped if no devices available or MFA not turned on in the cluster):

The second step is the actual password change; the data required depends on the verification method selected. For passkeys, we take advantage of strong identity verification and don't ask for the current password. This is the most important feature, and the actual fix for #36233. It allows passwordless users to set a new password easily. See RFD 0159 for detailed technical design.

For regular MFA devices or no verification at all, the current password is required.

For authenticator apps, we additionally require an OTP code.

To reviewers

This is quite a big change, so let me try to make it easier for you and provide some guidelines:

The backend changes add a specific challenge scope for setting passwords and allow setting a new password without giving an old one, provided that the user used a strong identity verification using a passwordless authentication device.
On the frontend side, the ChangePasswordWizard.tsx is the main chunk of new code, along with the accompanying test and story.
The backend interface got changed in auth.ts.
The rest of frontend changes is mostly wiring the new dialog with existing account settings view, and fixing types here and there. Most notably, the way we tell difference between passkeys and other devices was changed in MfaDevice. This was necessary to compute the list of identity verification options.
There's an upcoming tiny fix to the Enterprise part to address changes to MfaDevice type.

Changelog: Added a new password change wizard.
Changelog: Allowed changing password without providing the current one if the user verifies their identity using a passwordless key (see RFD 0159).

github-actions · 2024-02-28T14:51:36Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

zmb3 · 2024-02-28T21:09:12Z

I was able to successfully add a password to a passwordless user 👍

My only feedback is that I was caught by surprise by this screen:

My default reaction was to click the purple next button (without scanning the code, I didn't really read the instructions well). Then I go the next screen and it asked for a code and realized I had to go back and scan the QR code.

I feel like most TOTP workflows I've used show the QR code on the same screen where they ask you to enter the code, but I'm fine either way if this was the intent.

gzdunek

The code looks really solid 👍
I tested it and didn't find any issues.

I'd only recommend requesting a review for the backend changes from someone more familiar with the user auth (perhaps Alan?).

gzdunek · 2024-02-29T10:15:25Z

web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx

+export function createReauthOptions(
+  auth2faType: Auth2faType,
+  passwordlessEnabled: boolean,
+  devices: MfaDevice[]
+) {
+  const options: ReauthenticationOption[] = [];
+
+  if (passwordlessEnabled) {
+    options.push({ value: 'passwordless', label: 'Passkey' });
+  }
+
+  const mfaEnabled = auth2faType === 'on' || auth2faType === 'optional';
+
+  if (auth2faType === 'webauthn' || mfaEnabled) {
+    options.push({ value: 'mfaDevice', label: 'MFA Device' });
+  }
+
+  if (auth2faType === 'otp' || mfaEnabled) {
+    options.push({ value: 'otp', label: 'Authenticator App' });
+  }
+
+  const allowedMethods = {};
+  for (const d of devices) {
+    allowedMethods[reauthMethodForDevice(d)] = true;
+  }
+
+  return options.filter(o => o.value in allowedMethods);
+}
+
+function reauthMethodForDevice(d: MfaDevice): ReauthenticationMethod {
+  if (d.usage === 'passwordless') return 'passwordless';
+  return d.type === 'totp' ? 'otp' : 'mfaDevice';
+}


What do you think of merging these functions into a single function? To me it was quite hard at first to understand how the two sets of options interact with each other.

So for example check for passwordless would look like this:

if (passwordlessEnabled && devices.find(d => d.usage === 'passwordless')) { options.push({ value: 'passwordless', label: 'Passkey' }); }

It's more verbose, but also more explicit.

I wanted to avoid using find(), since technically, this is a collection of unknown size. But I agree this fragment is a bit cryptic, I'll change it.

gzdunek · 2024-02-29T10:45:17Z

web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx

+      <StepSlider
+        flows={wizardFlows}
+        currFlow={
+          reauthRequired ? 'withReauthentication' : 'withoutReauthentication'


I think we should hide step 1 of 1, it doesn't provide any information.

bl-nero · 2024-02-29T12:09:49Z

I feel like most TOTP workflows I've used show the QR code on the same screen where they ask you to enter the code, but I'm fine either way if this was the intent.

@zmb3 That screen is actually covered by the previous PRs (the ones that added AddAuthDeviceWizard), but since you brought it up: yes, I agree that it may be confusing, so let's see if our users have any strong opinions about it. The problem with OTPs, unfortunately, is that they are short-living by nature (30-60 seconds, to be precise). To make sure that they don't expire by the time they are consumed, they need to be in the last step (note that in the password flow, it's deliberately the very last field the user fills). Since the change that the UX team wanted to see is to set the auth method name after the auth method was configured, that's what we ended up with. Note that it can be changed, but this will require more work (either by implementing a more complex flow on the backend that includes renaming auth devices, or by cramming all authenticator device configuration into one step and then extending the StepSlider to support changing flows in the middle). That's why I'd rather release it as it is and look if we get any feedback.

If you'd like to discuss MFA/passkeys further, I propose to move the discussion somewhere else, because it's outside this PR's scope.

codingllama

Some initial high-level comments:

+1 for the new flow, looks good!

I feel like most TOTP workflows I've used show the QR code on the same screen where they ask you to enter the code, but I'm fine either way if this was the intent.

+1 to this comment by Zac, although I would suggest a follow up PR for it (if addressed at all).

The PR is a bit on the large side. I would split server-side and frontend changes into different PRs - that ought to make it much smaller and you can get "specialized" reviewers from each side.

I'd say keep this one for the FE changes, due to current reviewers and approvals, and "break out" the server-side PR from it.

bl-nero · 2024-02-29T14:31:01Z

3. The PR is a bit on the large side. I would split server-side and frontend changes into different PRs

@codingllama Unfortunately, the both are tightly coupled, because to change the server side, I'd also have to change the frontend interface, make the old UI use the new logic, and so on - I decided to take my chances early, because I counted on it not being as big. Well, it ended up being big. If you insist on merging these separately, I can make an attempt to split out the backend code, perhaps making the old frontend compatible will not be as hard as I thought.

codingllama · 2024-02-29T14:52:08Z

You can branch one PR from the other if they have a hard dependency.

…ssword

bl-nero · 2024-02-29T19:08:33Z

I've extracted the backend part to a separate PR.

kimlisa

LGTM, i just have nits that aren't blockers:

the sliding animation looks weird/squished because there is no padding on the component's meant for sliding between transitions. simple fix is to remove the padding on the dialog, and define padding for each component meant for sliding.

probably not the best way to do things, but it's the fastest without tinkering the stepslider (if we have time to tinker, we can maybe pass in a padding prop to stepslider so stepslider can add the padding to each component, but it's going to require adding this padding to the height calculations between transition)

for the account setting, for each dialog, the buttons should be bigger as depicted on figma? the same size as login form buttons but 🤷‍♀️

…ssword

bl-nero · 2024-03-04T17:28:48Z

@kimlisa Thanks for the feedback! Yes, the slider looks much better now. As for the buttons, I deliberately didn't tinker with them, since we'd like to take care of the buttons globally later this quarter.

…ssword

bl-nero · 2024-03-05T12:38:53Z

Merged with backend changes and retested manually:

Changing password with a passwordless MFA
Changing password with a YubiKey
Changing password with OTP device
Changing password without confirmation (user doesn't have any auth device).

All with both success and failure cases.

…ssword

bl-nero · 2024-03-06T13:03:58Z

Merged in the backend changes, retested manually:

Changing password (both in success and failure cases):
- Changing password with a passwordless MFA
- Changing password with a YubiKey
- Changing password with OTP device
- Changing password without confirmation (user doesn't have any auth device).
Signing in using the changed password and various MFA options.

public-teleport-github-review-bot · 2024-03-06T13:46:13Z

@bl-nero See the table below for backport results.

Branch	Result
branch/v15	Failed

codingllama · 2024-03-06T17:02:54Z

web/packages/teleport/src/services/mfa/makeMfaDevice.ts

  return {
    id,
    name,
    description,
    registeredDate: new Date(addedAt),
    lastUsedDate: new Date(lastUsed),
-    residentKey,


Note: dropping this field broke e/ builds.

Example: https://github.com/gravitational/teleport.e/actions/runs/8173863278/job/22347267175

yarn run v1.22.21 $ NODE_OPTIONS='--max-old-space-size=4096' tsc e/web/teleport/src/Recovery/RecoveryFlow/RecoveryFlow.test.tsx(7[5](https://github.com/gravitational/teleport.e/actions/runs/8173863278/job/22347267175#step:8:6),11): error TS2353: Object literal may only specify known properties, and 'residentKey' does not exist in type 'MfaDevice'. e/web/teleport/src/Recovery/RecoveryFlow/RecoveryFlow.test.tsx(83,11): error TS2353: Object literal may only specify known properties, and 'residentKey' does not exist in type 'MfaDevice'. error Command failed with exit code 2. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. Error: Process completed with exit code 2.

@bl-nero

public-teleport-github-review-bot · 2024-03-07T11:17:42Z

@bl-nero See the table below for backport results.

Branch	Result
branch/v15	Failed

public-teleport-github-review-bot · 2024-03-08T14:23:36Z

@bl-nero See the table below for backport results.

Branch	Result
branch/v15	Failed

* A new password change wizard * Review and lint * Allow password change with identity verificaiton * Review * Review * Revert renaming the webauthnAssertionResponse field * review * Fix a bug, add a test * Fix a broken test * Add license

A new password change wizard

fb2397d

github-actions bot added size/lg ui labels Feb 28, 2024

github-actions bot requested review from gzdunek and kimlisa February 28, 2024 14:51

bl-nero added the backport/branch/v15 label Feb 28, 2024

gzdunek approved these changes Feb 29, 2024

View reviewed changes

bl-nero requested a review from codingllama February 29, 2024 11:51

codingllama reviewed Feb 29, 2024

View reviewed changes

bl-nero added 3 commits February 29, 2024 16:24

Review and lint

58b1d71

Allow password change with identity verificaiton

4a62123

Merge branch 'bl-nero/change-password-backend' into bl-nero/change-pa…

78a849d

…ssword

bl-nero mentioned this pull request Feb 29, 2024

Allow password change with identity verification #38807

Merged

bl-nero changed the base branch from master to bl-nero/change-password-backend February 29, 2024 18:59

kimlisa approved these changes Mar 4, 2024

View reviewed changes

bl-nero added 2 commits March 4, 2024 18:00

Review

a632d0f

Merge branch 'bl-nero/change-password-backend' into bl-nero/change-pa…

083a3e3

…ssword

bl-nero added 6 commits March 4, 2024 18:29

Review

8f3da91

Revert renaming the webauthnAssertionResponse field

4879e16

review

7c13715

Fix a bug, add a test

5e48d12

Merge branch 'bl-nero/change-password-backend' into bl-nero/change-pa…

6796ef6

…ssword

Fix a broken test

3853fc1

Merge branch 'bl-nero/change-password-backend' into bl-nero/change-pa…

a9d2540

…ssword

Base automatically changed from bl-nero/change-password-backend to master March 5, 2024 14:10

bl-nero added 3 commits March 6, 2024 11:57

Merge branch 'master' into bl-nero/change-password-backend

520da0d

Merge branch 'bl-nero/change-password-backend' into bl-nero/change-pa…

413c2aa

…ssword

Merge branch 'master' into bl-nero/change-password

f9ceaa4

bl-nero enabled auto-merge March 6, 2024 13:04

Add license

860e8c0

bl-nero added this pull request to the merge queue Mar 6, 2024

Merged via the queue into master with commit 924ac92 Mar 6, 2024
33 checks passed

bl-nero deleted the bl-nero/change-password branch March 6, 2024 13:45

codingllama reviewed Mar 6, 2024

View reviewed changes

This was referenced Mar 8, 2024

[v15]: A new password change wizard #39124

Merged

Deprecate unused password challenge endpoints and remove them from the client code #39199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A new password change wizard #38728

A new password change wizard #38728

bl-nero commented Feb 28, 2024 •

edited

github-actions bot commented Feb 28, 2024

zmb3 commented Feb 28, 2024

gzdunek left a comment

gzdunek Feb 29, 2024

bl-nero Feb 29, 2024

gzdunek Feb 29, 2024

bl-nero commented Feb 29, 2024

codingllama left a comment

bl-nero commented Feb 29, 2024

codingllama commented Feb 29, 2024

bl-nero commented Feb 29, 2024

kimlisa left a comment

bl-nero commented Mar 4, 2024

bl-nero commented Mar 5, 2024

bl-nero commented Mar 6, 2024

public-teleport-github-review-bot bot commented Mar 6, 2024

codingllama Mar 6, 2024

public-teleport-github-review-bot bot commented Mar 7, 2024

public-teleport-github-review-bot bot commented Mar 8, 2024

A new password change wizard #38728

A new password change wizard #38728

Conversation

bl-nero commented Feb 28, 2024 • edited

What's this?

To reviewers

github-actions bot commented Feb 28, 2024

zmb3 commented Feb 28, 2024

gzdunek left a comment

Choose a reason for hiding this comment

gzdunek Feb 29, 2024

Choose a reason for hiding this comment

bl-nero Feb 29, 2024

Choose a reason for hiding this comment

gzdunek Feb 29, 2024

Choose a reason for hiding this comment

bl-nero commented Feb 29, 2024

codingllama left a comment

Choose a reason for hiding this comment

bl-nero commented Feb 29, 2024

codingllama commented Feb 29, 2024

bl-nero commented Feb 29, 2024

kimlisa left a comment

Choose a reason for hiding this comment

bl-nero commented Mar 4, 2024

bl-nero commented Mar 5, 2024

bl-nero commented Mar 6, 2024

public-teleport-github-review-bot bot commented Mar 6, 2024

codingllama Mar 6, 2024

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Mar 7, 2024

public-teleport-github-review-bot bot commented Mar 8, 2024

bl-nero commented Feb 28, 2024 •

edited