Recovery: Fix backup change detection not working in rust #12160
Conversation
BillCarsonFr
added
T-Task
BillCarsonFr
force-pushed
the
valere/fix_26792
branch
from
5904b1b
to
f0e34e4
Compare
BillCarsonFr
force-pushed
the
valere/fix_26792
branch
from
f0e34e4
to
f6b14ce
Compare
| @@ -1675,7 +1675,8 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> { | |||
| let haveNewVersion: boolean | undefined; | |||
| let newVersionInfo: IKeyBackupInfo | null = null; | |||
| // if key backup is still enabled, there must be a new backup in place | |||
| if (cli.getKeyBackupEnabled()) { | |||
| const backupEnabled = (await cli.getCrypto()!.getActiveSessionBackupVersion()) !== null; | |||
There was a problem hiding this comment.
Could you just put a Boolean() cast here if the variable is called backupEnabled, or if it's staying as string | null, maybe call the variable backupVersion?
There was a problem hiding this comment.
Udpated. The logic has changed a bit also.
| @@ -66,7 +65,7 @@ export default class NewRecoveryMethodDialog extends React.PureComponent<IProps> | |||
| const hackWarning = <p className="warning">{_t("encryption|new_recovery_method_detected|warning")}</p>; | |||
|
|
|||
| let content: JSX.Element | undefined; | |||
| if (MatrixClientPeg.safeGet().getKeyBackupEnabled()) { | |||
| if (this.props.newVersionInfo) { | |||
There was a problem hiding this comment.
this.props.newVersionInfo is of type IKeyBackupInfo. Won't that always be truthy?
There was a problem hiding this comment.
Right, this code was a bit confusing and I added a bit more.
So I looked at what it was doing, and updated it to make a bit more sense.
So the idea is that when we detect that the backup is removed/updated we want to notify the user.
I don't think it was that usefull to make a difference if the new backup is trusted or not as anyhow it was offering the option to go to settings. And from settings you can do what you want.
So basically there are 2 cases:
- There is no backup anymore: Shows the dialog to setup a new one as a shortcut or go to settings
- There is a new backup: Shows the dialog that notifies you with an option to open the settings.
I added playwrigth tests to test it for rust crypto.
Unfortunatly the detection of change on legacy crypto is harder to trigger.. I disabled the test for legacy for now.
I think that if we really want this feature, we should do it better and improve the code to detect backup changes
There was a problem hiding this comment.
The other option would be to remove that feature until we have clearer requirements
|
Closing for now as the requirements are not clear enough, and the current solution only partially detect changes |
Fixes element-hq/element-web#26792
React sdk was still making usage of
client#getKeyBackupEnabled()that is deprecated and failing in rust stack.That would cause the following dialog to not show up:
When the backup is deleted from another session
When backup is reset from another session
Checklist
Here's what your changelog entry will look like:
🐛 Bug Fixes