Allow user to control if they are signed out of all devices when changing password

[hughns]

Depends on matrix-org/matrix-js-sdk#2285

Attempts to resolve element-hq/element-web#2671 for Element Web.

Forgot password flow

If the HS does have the capability (supports r0.6.1) then we present the option of whether to sign out other devices:

And if the checkbox is ticked then we show a warning/confirmation modal:

If the HS does not have the capability then the behaviour is as it is now, but with a revised wording in the confirmation modal:

Change password from Settings

If the HS does have the capability or the user only has one device (the current one) then we proceed immediately to changing the password without signing out other devices:

Otherwise, we continue to show a warning/confirmation modal, but with some revised wording:

Notes:
Make sign out on all devices during password reset optional. Changing password in Settings does not sign out all devices.

---

Here's what your changelog entry will look like:

✨ Features

• Allow user to control if they are signed out of all devices when changing password (#8259). Fixes changing password should not log out all your devices (and destroy your e2e history) element-hq/element-web#2671. Contributed by @hughns.

Preview: https://pr8259--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

[hughns]

I'm not sure if this is considered a breaking change or not: the confirm property in the ChangePassword component has been removed.

If necessary I can reinstate confirm and if confirm=true then show the existing modal and actually sign out all devices.

[codecov]

Codecov Report

Merging #8259 (f3260a0) into develop (73e8387) will decrease coverage by 0.63%.
The diff coverage is 0.00%.

❗ Current head f3260a0 differs from pull request most recent head 9038ea7. Consider uploading reports for the commit 9038ea7 to get more accurate results

@@             Coverage Diff             @@
##           develop    vector-im/element-web#8259      +/-   ##
===========================================
- Coverage    30.42%   29.79%   -0.64%     
===========================================
  Files          889      874      -15     
  Lines        50544    50038     -506     
  Branches     12861    12738     -123     
===========================================
- Hits         15379    14908     -471     
+ Misses       35165    35130      -35     

Impacted Files	Coverage Δ
src/PasswordReset.ts	0.00% <0.00%> (ø)
src/components/structures/auth/ForgotPassword.tsx	0.00% <0.00%> (ø)
src/components/views/settings/ChangePassword.tsx	0.00% <0.00%> (ø)
...iews/settings/tabs/user/GeneralUserSettingsTab.tsx	0.00% <0.00%> (ø)
src/utils/colour.ts	0.00% <0.00%> (-94.12%)	⬇️
src/autocomplete/EmojiProvider.tsx	16.27% <0.00%> (-72.86%)	⬇️
src/components/views/elements/FacePile.tsx	10.00% <0.00%> (-67.78%)	⬇️
src/autocomplete/AutocompleteProvider.tsx	0.00% <0.00%> (-60.72%)	⬇️
src/components/views/elements/CopyableText.tsx	9.09% <0.00%> (-49.25%)	⬇️
src/stores/widgets/WidgetMessagingStore.ts	45.00% <0.00%> (-48.55%)	⬇️
... and 80 more

[hughns]

I've put a check box on the password reset form:

Only if the box is checked then a revised confirmation modal is shown:

[t3chguy]

Looks sane to me otherwise!

[hughns]

I've incorporated the wording suggestions into the latest commits.

The screens now look like this:

[Mikaela]

Will it really sign out all devices or all other devices (excluding this one)?

[hughns]

Will it really sign out all devices or all other devices (excluding this one)?

My thinking was: the current device isn't signed in at the moment (otherwise the user wouldn't need to reset the password).

However, I think either wording is valid:

• "Sign out all devices"

• "Sign out all other devices"

I would advocate for whichever we think is the clearest.

[dkasak]

Note that Element Web also has functionality to change the password while logged in. Does this PR only handle the password resetting case?

[hughns]

Note that Element Web also has functionality to change the password while logged in. Does this PR only handle the password resetting case?

Yes, as per release Note it changes the behaviour of change password in Settings so that it doesn't sign out all devices.

My rationale for this was that there is already the ability to sign out sessions from the Security & Privacy tab of Settings. So there is no need for duplicate functionality in change password.

As such, the implementation I have proposed is that there is no check box to sign out all devices on the change password function of Settings, and the corresponding warning dialog has also been removed.

If there is still felt to be a need to provide the option to sign out all sessions whilst changing password in Settings, or perhaps to sign post where the functionality has moved to, then I can add it.

[t3chguy]

We need a way of checking if the server will respect the flag, otherwise we're misleading the user in both cases (forgot and change password)

[hughns]

We need a way of checking if the server will respect the flag, otherwise we're misleading the user in both cases (forgot and change password)

This is a very good point.

[clokep]

Just an FYI that Synapse has supported this since v1.12.0 released on 2020-03-23 and it seems that it was added in r0.6.1 of the spec: https://matrix.org/docs/spec/client_server/r0.6.1.html#changelog

[t3chguy]

Sure but other server implementations may not support it, and promising the user something relating to security which doesn't work without warning is baaaad

[clokep]

Sure but other server implementations may not support it, and promising the user something relating to security which doesn't work without warning is baaaad

Of course, my point is that you should be able to key on the r0.6.1 version to know if it is supported!

[hughns]

A HS agnostic approach could be to use Capability negotiation and extend the m.change_password capability to say if this is supported or not.

[t3chguy]

Upon re-read, https://github.com/matrix-org/dendrite/blob/f2e550efd832662e6c032bebfef4a68da0b4c8ee/clientapi/routing/password.go#L37 isn't forced to true, but rather the default value. Dendrite supports this field.

[t3chguy]

And Conduit does not advertise 0.6.1 support, only 0.6.0 - so using 0.6.1 should be ok

[hughns]

I've added the logic to check the version (which depends on matrix-org/matrix-js-sdk#2297).

I checked it against matrix.org, dendrite.matrix.org, element.io and conduit.rs.

This implementations creates a temporary MatrixClient for the forgotten password flow to query the versions endpoint. It felt heavyweight but I couldn't see a pattern to follow.

js-sdk could be updated to cache (or similar) the results of the versions request it does during autodiscovery, but this would be more invasive.

[robintown]

Using this to change your password in settings results in a dialog that says "You will not receive push notifications on other sessions until you log back in to them", which I believe is no longer accurate

[hughns]

Using this to change your password in settings results in a dialog that says "You will not receive push notifications on other sessions until you log back in to them", which I believe is no longer accurate

Good spot.

I've pushed changes that take account of whether all devices were signed out during the password change.

If the server supports not signing out all devices:

Otherwise:

[hughns]

Whilst making these last changes I realised that in the case the user only has one logged in device we could simplify the wording throughout. i.e. we don't need to warn them when there aren't any other devices that would actually be signed out.

I don't think it would be hard to roll into this change. Good idea? Objections?

[hughns]

Whilst making these last changes I realised that in the case the user only has one logged in device we could simplify the wording throughout. i.e. we don't need to warn them when there aren't any other devices that would actually be signed out.

I don't think it would be hard to roll into this change. Good idea? Objections?

I've gone ahead and made these changes. I've put the full screenshots in the summary of this PR for ease of review.

[nadonomy]

@hughns as functional changes these LGTM, so approving on behalf of design.

There's a lot we could improve in the UX copy (too much jargon, not layperson friendly) but a lot of that copy is already on production predating this PR.

Anticipating we'll improve this in earnest when we make larger changes to reset password (OIDC/auth?).