-
-
Notifications
You must be signed in to change notification settings - Fork 616
Fix RustCrypto.resetEncryption
failure
#4772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, one request 👍
@@ -1466,6 +1466,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, CryptoEventH | |||
await this.backupManager.deleteAllKeyBackupVersions(); | |||
|
|||
this.deleteSecretStorage(); | |||
await this.deleteSecretStorage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the test so this would be caught by it please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is difficult because we are using a mocked SecretStorage which is way faster than the real execution. Meaning that even by adding jest.fn().mockResolvedValue(undefined)
to store
or setDefaultKeyId
, the test passes. Also I don't think that adding manual latency is a good thing neither.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'd prefer the change to mockResolvedValue
for async methods, even if it doesn't make the test fail. Personally I would add a small delay that makes the test fail but I definitely won't insist on that.
I'll tick the PR and you can use your judgement. Thank you!
this.deleteSecretStorage(); | ||
await this.deleteSecretStorage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@florianduros: Why do we need to do this.deleteSecretStorage()
and await this.deleteSecretStorage();
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, an error during the rebase. Good spot, I'll fix it tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in #4789
Regression due to #4742
When
RustCrypto.resetEncryption
is called,CrossSigningIdentity.resetCrossSigning
raises this error:The 4S is not completely deleted and we try to export keys that has been deleted. Due to of missing
await
onthis.deleteSecretStorage()
inmatrix-js-sdk/src/rust-crypto/rust-crypto.ts
Lines 1458 to 1480 in 3657eb6
The secretStorage object is mocked in the test, the mock making it synchronous so the test didn't catch the race.