Skip to content
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

Support for e2e key backups #736

Merged
merged 85 commits into from
Nov 21, 2018
Merged

Support for e2e key backups #736

merged 85 commits into from
Nov 21, 2018

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Sep 18, 2018

Adds support for backing up e2e keys to the homeserver and restoring with a secret recovery key.

element-hq/element-web#3661

ara4n and others added 30 commits January 15, 2018 01:50
Continues from uhoreg's branch
Add method to check if a given string is a valid recovery key
Remove commented code block as it's not immediately obvious it makes
sense or is the right way of suggesting a key restore.
this._baseApis.enableKeyBackup(backupInfo);
} else if (!trustInfo.usable && this.backupInfo) {
console.log("No usable key backup: disabling key backup");
this._baseApis.disableKeyBackup();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not managing the case (trustInfo.usable && this.backupInfo) ?

If I understand the logic right, it would be:

else if (trustInfo.usable && this.backupInfo) {
        console.log("Found usable key backup: use the new key backup version");
        this._baseApis.disableKeyBackup();
        this._baseApis.enableKeyBackup(backupInfo);
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, we should do nothing if the backup version we got from the hs is the same as in this.backupInfo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry - replied to this on matrix but should do so here for completeness: we probably ought to but there's nothing that would actually trigger us to look for this case since we don't get told about new backup version in the sync stream.

@ara4n
Copy link
Member

ara4n commented Nov 9, 2018

lgtm. (turning the 43 and 44 magic numbers into a const is nice-to-have)

to force base-x to version 3.0.4 because 3.0.5 breaks the build
by exporting ES6.
}
}

if (
Copy link
Contributor

@manuroe manuroe Nov 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Length could be checked first to avoid out of bounds error in the for loop before this piece of code.

(Credits to Benoit at matrix-org/matrix-ios-sdk#591 (comment))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I don't think it matters much for JavaScript, but it would matter for lower-level languages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants