[native] Don't bother with keyserver auth in BackgroundIdentityLoginHandler
ClosedPublic
Actions

Authored by ashoat on Jul 6 2024, 2:39 PM.

Details

Reviewers

varun
tomek

Commits

rCOMM1419a4bd0741: [native] Don't bother with keyserver auth in BackgroundIdentityLoginHandler

Summary

We don't need to do it here, since we're already auth'd with the authoritative keyserver.

We just need to perform identity auth.

This mitigates ENG-8725 and ENG-8726, and addresses ENG-8779.

Test Plan

Confirm that I'm able to go from a release build running with usingCSAT=false to one running with usingCSAT=true. Exact instructions here. Make sure that the two "mitigated" issues are no longer present, and that the upgrade goes without issue (except for ENG-8778, which is a separate issue)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Jul 6 2024, 2:39 PM

ashoat added a child revision: D12679: [native] Limit BackgroundIdentityLoginHandler to one try per startup.

Harbormaster completed remote builds in B30174: Diff 42085.Jul 6 2024, 2:56 PM

ashoat requested review of this revision.Jul 6 2024, 2:56 PM

we're already auth'd with the authoritative keyserver

Why is it guaranteed?

This revision is now accepted and ready to land.Jul 8 2024, 1:40 AM

In D12678#359222, @tomek wrote:

we're already auth'd with the authoritative keyserver

Why is it guaranteed?

That's a good question. The dataLoaded condition generally guarantees this, but I'm currently investigating a situation in ENG-8772 where dataLoaded is true despite the user being logged out (currentUserInfo is anonymous).

As part of that investigation, I'll probably conclude that we need to block BackgroundIdentityLoginHandler on both conditions in LogInHandler: loggedIn and navLoggedIn. I've created ENG-8785, and we can track this work there.