Avoid key prompts when resetting crypto #4586
Conversation
Attempting to get the backup key out of secret storage can cause the user to be prompted for their key, which is not helpful if this is being done as part of a reset. This check was redundant anyway and we can just overwrite the key with the same value. Also fix docs and remove check for active backup.
|
AIUI this fixes a regression introduced in #4542? Could we get them combined in the changelog? |
| if (backupKeyFromStorage !== backupKeyBase64) { | ||
| await this.secretStorage.store("m.megolm_backup.v1", backupKeyBase64); | ||
| } | ||
| await this.secretStorage.store("m.megolm_backup.v1", backupKeyBase64); |
There was a problem hiding this comment.
The documentation on this method says "... and we have secret storage active ..." - is that correct?
There was a problem hiding this comment.
Mmm, doesn't look like there's a way to determine if we have 4s set up, afaics, so I guess I'll just remove that bit.
There was a problem hiding this comment.
There is this.secretStorageHasAESKey() which is what resetKeyBackup does, but given this is only called from bootstrapSecretStorage, I don't think there's any need to call it.
I don't think we have a way of doing that automatically, but we could annotate it and flag it for does the release, although as far as what will do in the changelog, I don't believe it would be any different?
Behaviour that specific didn't have its own tests, no. |
It's a bit of a hobbyhorse of mine, but ideally a changelog would list all the PRs that contributed to making a particular change. In other words, if it takes a few PRs to land the complete feature and shake out the bugs, they all get listed and linked under one entry in the changelog, rather than dotted throughout the changelog. The Synapse changelog does quite a good job of this, for example:
|
|
Ah, I see. Unfortunately I don't think that's a thing supported by https://github.com/t3chguy/release-drafter as far as I can see. |
Attempting to get the backup key out of secret storage can cause the user to be prompted for their key, which is not helpful if this is being done as part of a reset. This check was redundant anyway and we can just overwrite the key with the same value.
Also fix docs and remove check for active backup.
Checklist
public/exportedsymbols have accurate TSDoc documentation.