-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix logic in DeviceListener #30230
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
Fix logic in DeviceListener #30230
Conversation
SETUP_ENCRYPTION tries to set up everything (4S, cross-signing and key backup), rather than just setting up encryption, as its name would imply. crossSigningReady == false happens when the user's device isn't verified, so it should trigger VERIFY_THIS_SESSION rather than SETUP_ENCRYPTION
rather than falling back to the SETUP_ENCRYPTION catch-all. Also, remove SETUP_ENCRYPTION since it is no longer used.
(almost) all the other functions that use make decisions based on Kind use switch statements
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.
Looks good. I answered my own question later in the review, so I think it all makes sense.
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.
Okay, think this looks sensible, thanks.
3d56aa7
}); | ||
showSetupEncryptionToast(SetupKind.SET_UP_ENCRYPTION); | ||
// If we get here, then we are verified, have key backup, and | ||
// 4S, but crypto.isCrossSigningReady returned false, which |
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.
crypto.isCrossSigningReady returned false
Are you sure this is the only way we can get here?
For example, what happens if isCrossSigningReady
returns true, but isSecretStorageReady
returns false?
In any case, please can you keep the logic to log all the flags, so that we can debug why we are falling into the else
case?
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.
Ugh. I typo'ed the comment. It should say "but crypto.isSecretStorageReady returned false". (If isCrossSigningReady
returns false, then isCurrentDeviceTrusted
will be false and we'll run the first branch of the if
statement.) I'll make a new PR to fix the comment and reinstate the logging.
Fixes #29883
SETUP_ENCRYPTION tried to set up all of encryption (4S, cross-signing, key backup), but we now have it split into different stages, so it is obsolete.
Review commit-by-commit
Checklist
public
/exported
symbols have accurate TSDoc documentation.