element-web icon indicating copy to clipboard operation
element-web copied to clipboard

EW still prompts you to "Set up Secure Backup" and "Enter a security phrase"

Open richvdh opened this issue 7 months ago • 12 comments

We're phasing out the term "security phrase", and indeed the ability to choose your own secret (cf https://github.com/element-hq/element-meta/issues/2394). Also, this is about setting up recovery, not "secure backup".

Steps to reproduce

  1. Register a new account
  2. Create a new room
  3. Observe "Set up Recovery" toast
  4. Click "Continue" on the toast

Image

richvdh avatar Jun 12 '25 20:06 richvdh

Yes, this dialog should go away, and the toast from https://github.com/element-hq/element-web/issues/29232 should open Settings -> Encryption instead of a separate dialog.

andybalaam avatar Jun 16 '25 13:06 andybalaam

This dialog comes from CreateSecretStorageDialog, which is called by accessSecretStorage in SecurityManager.ts. So I should also update CreateSecretStorageDialog to use the new designs (probably by replacing the contents of the dialog with the components from the Encryption settings), in case it gets called in some other way.

@mxandreas do you still want the toast to open Settings -> Encryption, or should I have it open the dialog with the new designs?

uhoreg avatar Jun 16 '25 21:06 uhoreg

Oh, CreateSecretStorageDialog is a fun file. I may take this opportunity to do some refactoring...

uhoreg avatar Jun 16 '25 21:06 uhoreg

do you still want the toast to open Settings -> Encryption, or should I have it open the dialog with the new designs?

Open Settings -> Encryption. Because then it is consistent with EX. But also, because (I assume) it would not require anything else to be done about the legacy modals (besides just removing the code code).

So I should also update CreateSecretStorageDialog to use the new designs (probably by replacing the contents of the dialog with the components from the Encryption settings), in case it gets called in some other way.

I do not think so, because there should not be any other way to call it (because we decoupled setting up recovery from reset).

mxandreas avatar Jun 17 '25 05:06 mxandreas

I do not think so, because there should not be any other way to call it (because we decoupled setting up recovery from reset).

It's called by the accessSecretStorage function, which is called in several places (basically, any time Element wants to do anything with Secret Storage). I'd like to say that the old design shouldn't show up, but the only way to make sure is to replace it. I also don't want to keep CreateSecretStorageDialog around if it isn't being used, because it contains logic that makes future maintenance harder.

uhoreg avatar Jun 17 '25 14:06 uhoreg

I've done a survey of the places where accessSecretStorage is called. Some of them call it when Secret Storage should already be set up, so shouldn't call CreateSecretStorageDialog. Some of them call it in order to get accessSecretStorage to create Secret Storage/key backup/cross-signing keys. Most of those can be converted to bringing the user to the Encryption settings tab instead. But two places that are problematic are:

  • NewRecoveryMethodDialog, calling RestoreKeyBackupDialog. To trigger this:
    1. open two instances of Element, logged into the same account.
    2. In instance 1, set up key backup and recovery.
    3. In instance 2, turn off key backup and then turn it back on.
    4. In instance 1, in an encrypted room, type /discardsession, and then send a message.
    5. After a bit (Element will try to upload the key to backup, and then fail), a dialog will pop up saying that there is a "New Recovery Method" Image
    6. Click on "Set Up Secure Messages" button, which will pop up a new dialog behind(!) the "New Recovery Method" dialog
    7. Click on the dialog to bring it to the front
    8. Click on "set up new recovery options" at the bottom, which will bring up the "Set up Secure Backup" dialog Here, we could bring the user to the Encryption settings page, but in order to create a new backup, the user needs to turn off Key storage, and then turn it on again, which isn't very intuitive. Moreover, that page just shows that Key storage is enabled, but doesn't give any indication that the client isn't able to use it. I think that we may still want to bring the user to the Encryption settings page, but improve that page to better handle the case where Key storage (and/or Recovery) exists, but the client isn't using it (e.g. show the correct state, and allow the user to enter their key). (Note that if in step 3, you also turn on Recovery, then the "New Recovery Method" will bring you to a different dialog, that asks you to enter your recovery key, but doesn't give the option of resetting)
  • verification.ts has a function for verifying another user verifyUser that uses accessSecretStorage to create the cross-signing keys if they don't exist. I'm not sure if it's possible, nowadays, to reach that state without having cross-signing keys, so we might be able to just get rid of that bit?

uhoreg avatar Jun 18 '25 20:06 uhoreg

I think that we may still want to bring the user to the Encryption settings page, but improve that page to better handle the case where Key storage (and/or Recovery) exists, but the client isn't using it (e.g. show the correct state, and allow the user to enter their key).

I have the following questions about this:

  1. How would we show the correct state? I think showing that the "Key Storage" is OFF, would be very confusing since for the user it is their global setting which they can simply turn on/off from any client. I am afraid we have to find a way how to avoid the need to make user aware of such state (e.g. one of their clients isn't able to use backup) - or we need to do it with the "key storage out of sync" approach that we already have.
  2. Besides the potential warning about "somebody created a new backup", what else do we need from the user? Do we need to them to enter the recovery key? (In your scenario, did you also set up recovery again in instance 2?)

mxandreas avatar Jun 19 '25 05:06 mxandreas

improve that page to better handle the case where Key storage (and/or Recovery) exists, but the client isn't using it

Yes, this would be an excellent way to help users who have got into this invalid state, which can be done quite easily.

andybalaam avatar Jun 19 '25 09:06 andybalaam

https://github.com/element-hq/element-web/issues/29171 tracks some problems with the NewRecoveryMethodDialog.

richvdh avatar Jun 19 '25 10:06 richvdh

I have the following questions about this:

  1. How would we show the correct state? I think showing that the "Key Storage" is OFF, would be very confusing since for the user it is their global setting which they can simply turn on/off from any client. I am afraid we have to find a way how to avoid the need to make user aware of such state (e.g. one of their clients isn't able to use backup) - or we need to do it with the "key storage out of sync" approach that we already have.

In theory, instead of showing the toggle, we could show a warning that it's not connected to key backup, and show maybe three buttons: one to create a disable backup, one to create a new backup, and one to connect to key backup (by entering the key backup key). However, we now don't show the user the key backup key when we create it, so the user doesn't have anything to enter.

So, it's a pretty terrible situation, and related to the secrets synchronisation thing that we were talking about during design hour. Basically, if you have two clients open, and you create a new Key backup in one client, but not Recovery, then we currently have no way of getting the backup key to the second client. Which means that if you have two clients, create Key backup in client 1 and then log out, and then log into a new client and verify against client 2, then client 3 can't open the Key backup, and will have UTDs. And also, if you have two clients open, and want to create a new key backup, then there's no way to have both clients use the key backup at the same time.

One solution to this would be to send the key backup key to all other clients when we create a new key backup (and change clients to accept unsolicited secrets from cross-signed devices), which is something that we discussed during the secrets synchronisation meetings.

  1. Besides the potential warning about "somebody created a new backup", what else do we need from the user? Do we need to them to enter the recovery key? (In your scenario, did you also set up recovery again in instance 2?)

Since there is no recovery key, the user can't enter that. If we showed the key backup key when we created it, the user might be able to enter that (and that's what we currently prompt for), but we don't show it any more.

uhoreg avatar Jun 19 '25 13:06 uhoreg

https://github.com/element-hq/element-web/issues/29170 tracks the issue of the Encryption settings dialog being confusing if backup is created by a different client and we don't have the private key

uhoreg avatar Jun 19 '25 14:06 uhoreg

After discussion, we decided that I would convert all of the places that call accessSecretStorage to set up Secret Storage to instead bring the user to the Encryption settings page, except for the NewRecoveryMethod dialog, which is tracked by https://github.com/element-hq/element-web/issues/29171

uhoreg avatar Jun 19 '25 15:06 uhoreg