fix(js): treat session notifications as hints, read the daemon's state

shared/constants/init/shared.test.ts

@@ -1,9 +1,11 @@
 /// <reference types="jest" />
 import * as T from '@/constants/types'
 import {resetAllStores} from '@/util/zustand'
+import {RPCError} from '@/util/errors'
 import {useConfigState} from '@/stores/config'
+import {useCurrentUserState} from '@/stores/current-user'
 import {useDaemonState} from '@/stores/daemon'
-import {loadAccountsStep} from './shared'
+import {_onEngineIncoming, initSharedSubscriptions, loadAccountsStep, onNetworkOnlineChanged} from './shared'
 
 describe('loadAccountsStep', () => {
   const originalDispatch = useConfigState.getState().dispatch
@@ -65,3 +67,241 @@ describe('loadAccountsStep', () => {
     expect(useConfigState.getState().configuredAccounts.map(a => a.username)).toEqual(['testuser'])
   })
 })
+
+describe('onNetworkOnlineChanged', () => {
+  const originalDaemonDispatch = useDaemonState.getState().dispatch
+  afterEach(() => {
+    jest.restoreAllMocks()
+    useDaemonState.setState({dispatch: originalDaemonDispatch})
+    resetAllStores()
+  })
+
+  const spyOnReRead = () => {
+    // userSwitching survives resetAllStores on purpose, and an earlier test in this file sets it
+    useConfigState.getState().dispatch.setUserSwitching(false)
+    const reRead = jest.fn()
+    useDaemonState.setState({
+      dispatch: {...originalDaemonDispatch, refreshSessionFromDaemon: reRead},
+      handshakeState: 'done',
+    })
+    return reRead
+  }
+
+  test('re-reads the session when the network comes back', () => {
+    const reRead = spyOnReRead()
+    onNetworkOnlineChanged(true, false)
+    expect(reRead).toHaveBeenCalledTimes(1)
+  })
+
+  test('does not re-read on the first reading of the network at startup', () => {
+    const reRead = spyOnReRead()
+    onNetworkOnlineChanged(true, undefined)
+    expect(reRead).not.toHaveBeenCalled()
+  })
+
+  test('does not re-read when going offline', () => {
+    const reRead = spyOnReRead()
+    onNetworkOnlineChanged(false, true)
+    expect(reRead).not.toHaveBeenCalled()
+  })
+
+  test('does not re-read during an account switch', () => {
+    const reRead = spyOnReRead()
+    useConfigState.getState().dispatch.setUserSwitching(true)
+    onNetworkOnlineChanged(true, false)
+    expect(reRead).not.toHaveBeenCalled()
+  })
+
+  test('does not re-read before the handshake is done', () => {
+    const reRead = spyOnReRead()
+    useDaemonState.setState({handshakeState: 'loading'})
+    onNetworkOnlineChanged(true, false)
+    expect(reRead).not.toHaveBeenCalled()
+  })
+})
+
+describe('the session comes from the daemon; notifications only say to read it', () => {
+  const status = (over: Partial<T.RPCGen.BootstrapStatus> = {}) =>
+    ({
+      deviceID: 'd1',
+      deviceName: 'testuser-mac',
+      fullname: '',
+      loggedIn: true,
+      registered: true,
+      uid: 'u1',
+      username: 'testuser',
+      ...over,
+    }) as unknown as T.RPCGen.BootstrapStatus
+  const userA = status()
+  const userB = status({deviceID: 'd2', uid: 'u2', username: 'testuser2'})
+  const loggedOut = status({deviceID: '', deviceName: '', loggedIn: false, uid: '', username: ''})
+
+  let replies: Array<(bs: T.RPCGen.BootstrapStatus) => void> = []
+  const flush = async () => jest.advanceTimersByTimeAsync(0)
+  const notify = (type: string, params: unknown) => _onEngineIncoming({payload: {params}, type} as never)
+  const readReplying = async (bs: T.RPCGen.BootstrapStatus) => {
+    useDaemonState.getState().dispatch.refreshSessionFromDaemon('test')
+    await flush()
+    replies[replies.length - 1]?.(bs)
+    await flush()
+  }
+
+  // what resetAllStores clears, standing in for the previous account's state
+  const markAccountState = () => useConfigState.setState({justDeletedSelf: 'testuser'})
+  const accountStateCleared = () => useConfigState.getState().justDeletedSelf === ''
+  const loginChanges = () => {
+    const changes: Array<boolean> = []
+    const unsub = useConfigState.subscribe((st, prev) => {
+      if (st.loggedIn !== prev.loggedIn) {
+        changes.push(st.loggedIn)
+      }
+    })
+    return {changes, unsub}
+  }
+
+  beforeEach(() => {
+    jest.useFakeTimers()
+    replies = []
+    jest.spyOn(T.RPCGen, 'configGetBootstrapStatusRpcPromise').mockImplementation(
+      async () =>
+        new Promise<T.RPCGen.BootstrapStatus>(resolve => {
+          replies.push(resolve)
+        })
+    )
+    jest.spyOn(T.RPCGen, 'loginGetConfiguredAccountsRpcPromise').mockResolvedValue([])
+    useConfigState.getState().dispatch.setUserSwitching(false)
+    initSharedSubscriptions()
+  })
+  afterEach(() => {
+    jest.useRealTimers()
+    jest.restoreAllMocks()
+    useConfigState.getState().dispatch.setUserSwitching(false)
+    resetAllStores()
+  })
+
+  test('loggedOut and loggedIn delivered reversed still end with the last reply', async () => {
+    // the service logged out and then in; its two notifications reached us the other way round
+    notify('keybase.1.NotifySession.loggedIn', {signedUp: false, username: 'testuser'})
+    notify('keybase.1.NotifySession.loggedOut', undefined)
+    await flush()
+    replies[1]?.(userA)
+    await flush()
+    replies[0]?.(loggedOut)
+    await flush()
+
+    expect(useConfigState.getState().loggedIn).toBe(true)
+    expect(useCurrentUserState.getState().username).toBe('testuser')
+  })
+
+  test('a loggedOut hint logs out once the daemon says so', async () => {
+    await readReplying(userA)
+    expect(useConfigState.getState().loggedIn).toBe(true)
+
+    notify('keybase.1.NotifySession.loggedOut', undefined)
+    expect(useConfigState.getState().loggedIn).toBe(true)
+    await flush()
+    replies[replies.length - 1]?.(loggedOut)
+    await flush()
+
+    expect(useConfigState.getState().loggedIn).toBe(false)
+  })
+
+  test('an http server update applies at once without re-reading the daemon', async () => {
+    const before = replies.length
+    notify('keybase.1.NotifyService.HTTPSrvInfoUpdate', {info: {address: '127.0.0.1:2', token: 'token'}})
+    expect(useConfigState.getState().httpSrv).toEqual({address: '127.0.0.1:2', token: 'token'})
+    await flush()
+    expect(replies.length).toBe(before)
+  })
+
+  test('a reply for another user while logged in logs out first, clearing the old account', async () => {
+    await readReplying(userA)
+    markAccountState()
+    const {changes, unsub} = loginChanges()
+
+    await readReplying(userB)
+    unsub()
+
+    expect(changes).toEqual([false, true])
+    expect(accountStateCleared()).toBe(true)
+    expect(useCurrentUserState.getState().uid).toBe('u2')
+    expect(useCurrentUserState.getState().username).toBe('testuser2')
+    expect(useDaemonState.getState().bootstrapStatus?.uid).toBe('u2')
+  })
+
+  test('the same user again is not a switch', async () => {
+    await readReplying(userA)
+    markAccountState()
+    const {changes, unsub} = loginChanges()
+
+    await readReplying({...userA, fullname: 'changed'} as T.RPCGen.BootstrapStatus)
+    unsub()
+
+    expect(changes).toEqual([])
+    expect(accountStateCleared()).toBe(false)
+  })
+
+  test('logged in with no current user yet is not a switch', async () => {
+    useConfigState.getState().dispatch.setLoggedIn(true)
+    markAccountState()
+    const {changes, unsub} = loginChanges()
+
+    await readReplying(userA)
+    unsub()
+
+    expect(changes).toEqual([])
+    expect(accountStateCleared()).toBe(false)
+    expect(useCurrentUserState.getState().uid).toBe('u1')
+  })
+
+  test('during an account switch a logged-out reply is ignored, and the new user still replaces the old', async () => {
+    await readReplying(userA)
+    markAccountState()
+    useConfigState.getState().dispatch.setUserSwitching(true)
+    const {changes, unsub} = loginChanges()
+
+    await readReplying(loggedOut)
+    expect(useConfigState.getState().loggedIn).toBe(true)
+
+    await readReplying(userB)
+    unsub()
+
+    expect(changes).toEqual([false, true])
+    expect(accountStateCleared()).toBe(true)
+    expect(useCurrentUserState.getState().username).toBe('testuser2')
+    expect(useConfigState.getState().userSwitching).toBe(true)
+  })
+
+  test('a switch whose login fails ends logged out, no longer switching', async () => {
+    await readReplying(userA)
+    useConfigState.getState().dispatch.setUserSwitching(true)
+    await readReplying(loggedOut)
+
+    useConfigState.getState().dispatch.setLoginError(new Error('bad password') as never)
+    await readReplying(loggedOut)
+
+    expect(useConfigState.getState().loggedIn).toBe(false)
+    expect(useConfigState.getState().userSwitching).toBe(false)
+  })
+
+  test.each([
+    ['cancelled', new RPCError('Canceling RPC', T.RPCGen.StatusCode.scgeneric)],
+    ['ended by a non-RPC error', new Error('engine reset')],
+  ])('a switch whose login is %s ends logged out, no longer switching', async (_, error) => {
+    await readReplying(userA)
+    useConfigState.getState().dispatch.setUserSwitching(true)
+    await readReplying(loggedOut)
+    expect(useConfigState.getState().loggedIn).toBe(true)
+
+    jest.spyOn(T.RPCGen, 'loginLoginRpcListener').mockRejectedValue(error)
+    useConfigState.getState().dispatch.login('testuser2', 'password')
+    await flush()
+
+    expect(useConfigState.getState().userSwitching).toBe(false)
+    expect(useConfigState.getState().loggedIn).toBe(false)
+    replies[replies.length - 1]?.(loggedOut)
+    await flush()
+    expect(useConfigState.getState().loggedIn).toBe(false)
+    expect(useConfigState.getState().loginError).toBeUndefined()
+  })
+})

shared/constants/init/shared.tsx

@@ -176,22 +176,20 @@ const onGregorPushStateChanged = (
   )
 }
 
-const onGregorReachableChanged = (gregorReachable: ConfigState['gregorReachable']) => {
-  // Re-get info about our account if you log in/we're done handshaking/became reachable
-  if (
-    gregorReachable === T.RPCGen.Reachable.yes &&
-    useDaemonState.getState().handshakeState === 'done' &&
-    !useConfigState.getState().userSwitching
-  ) {
-    ignorePromise(useDaemonState.getState().dispatch.loadDaemonBootstrapStatus())
+// After an offline stretch, reread the session to pick up what the service learned while we could
+// not reach it. `previous === undefined` is the first reading of the network at startup, which the
+// handshake's own read already covers.
+export const onNetworkOnlineChanged = (online?: boolean, previous?: boolean) => {
+  if (!online || previous !== false) {
+    return
+  }
+  if (useDaemonState.getState().handshakeState === 'done' && !useConfigState.getState().userSwitching) {
+    useDaemonState.getState().dispatch.refreshSessionFromDaemon('back online')
   }
 }
 
 const onLoggedInChanged = (loggedIn: ConfigState['loggedIn']) => {
   if (loggedIn) {
-    // runtime login: refresh bootstrap status. During the handshake this is already in
-    // flight, and the store dedupes it.
-    ignorePromise(useDaemonState.getState().dispatch.loadDaemonBootstrapStatus())
     scheduleStartupOrReloginWork()
   } else {
     clearSignupEmail()
@@ -221,23 +219,45 @@ const onBootstrapStatusChanged = (bootstrap: DaemonState['bootstrapStatus']) =>
   }
 
   const {deviceID, deviceName, loggedIn, uid, username} = bootstrap
-  useCurrentUserState.getState().dispatch.setBootstrap({deviceID, deviceName, uid, username})
-
   const configDispatch = useConfigState.getState().dispatch
-  if (username) {
-    configDispatch.setDefaultUsername(username)
-  }
+
+  // Before the identity: the user we hold is what tells the new account's session from the old.
+  // onUserSwitchingChanged applies the status once the switch ends.
   if (!loggedIn && useConfigState.getState().userSwitching) {
     logger.info('[Bootstrap] ignoring loggedIn=false result during account switch')
     return
   }
+
+  // Logged in as someone else than the user we hold is a logout and then a login, however the
+  // notifications in between reached us. Logging out clears the previous account's stores, the
+  // daemon's status among them, so put this status back and let that change apply it.
+  const currentUid = useCurrentUserState.getState().uid
+  if (loggedIn && useConfigState.getState().loggedIn && currentUid && uid !== currentUid) {
+    logger.info('[Bootstrap] the session is another user now, logging out the previous one')
+    configDispatch.setLoggedIn(false)
+    useDaemonState.getState().dispatch.setBootstrapStatus(bootstrap)
+    return
+  }
+
+  useCurrentUserState.getState().dispatch.setBootstrap({deviceID, deviceName, uid, username})
+  if (username) {
+    configDispatch.setDefaultUsername(username)
+  }
   configDispatch.setLoggedIn(loggedIn)
 
   if (bootstrap.httpSrvInfo) {
     configDispatch.setHTTPSrvInfo(bootstrap.httpSrvInfo.address, bootstrap.httpSrvInfo.token)
   }
 }
 
+// A switch that failed after the service logged out has a logged-out status nothing applied, and
+// a read after the switch returns the same status, which does not count as a change.
+const onUserSwitchingChanged = (userSwitching: ConfigState['userSwitching']) => {
+  if (!userSwitching) {
+    onBootstrapStatusChanged(useDaemonState.getState().bootstrapStatus)
+  }
+}
+
 // Native reports the app state from the same callbacks that report it to Go, and this is the only
 // writer of mobileAppState. Desktop has no lifecycle; its window focus goes straight to appFocused.
 export const applyMobileAppState = (state: AppLifecycleState) => {
@@ -325,7 +345,7 @@ export const onEngineConnected = () => {
             chatattachments: true, chatdev: false, chatemoji: false, chatemojicross: false, chatkbfsedits: false,
             deviceclone: false, ephemeral: false, favorites: false, featuredBots: false, kbfs: true, kbfsdesktop: !isMobile,
             devicehistory: true, kbfslegacy: false, kbfsrequest: false, kbfssubscription: true, keyfamily: false, notifysimplefs: true,
-            paperkeys: false, pgp: true, reachability: true, runtimestats: true, saltpack: true, service: true, session: true,
+            paperkeys: false, pgp: true, reachability: false, runtimestats: true, saltpack: true, service: true, session: true,
             team: true, teambot: false, tracking: true, users: true, wallet: false,
           },
         })
@@ -361,14 +381,15 @@ export const initSharedSubscriptions = (platformBootstrapSteps: Array<BootstrapS
   for (const unsub of _sharedUnsubs) unsub()
   _sharedUnsubs.length = 0
   _sharedUnsubs.push(
-    subscribeValue(useConfigState, s => s.gregorReachable, onGregorReachableChanged),
     subscribeValue(useConfigState, s => s.gregorPushState, onGregorPushStateChanged),
     subscribeValue(useConfigState, s => s.loggedIn, onLoggedInChanged),
     subscribeValue(useConfigState, s => s.revokedTrigger, onRevokedTriggerChanged),
-    subscribeValue(useConfigState, s => s.configuredAccounts, onConfiguredAccountsChanged)
+    subscribeValue(useConfigState, s => s.configuredAccounts, onConfiguredAccountsChanged),
+    subscribeValue(useConfigState, s => s.userSwitching, onUserSwitchingChanged)
   )
 
   _sharedUnsubs.push(subscribeValue(useDaemonState, s => s.bootstrapStatus, onBootstrapStatusChanged))
+  _sharedUnsubs.push(subscribeValue(useShellState, s => s.networkStatus?.online, onNetworkOnlineChanged))
 
   _sharedUnsubs.push(
     subscribeValue(useRouterState, s => s.navState, onNavStateChanged)
@@ -388,6 +409,12 @@ export const _onEngineIncoming = (action: EngineGen.Actions) => {
   }
 
   switch (action.type) {
+    // These can reach us out of order with each other, so none of them sets the session: each only
+    // says it changed, and the daemon's reply to the latest read is what applies.
+    case 'keybase.1.NotifySession.loggedIn':
+    case 'keybase.1.NotifySession.loggedOut':
+      useDaemonState.getState().dispatch.refreshSessionFromDaemon(action.type)
+      break
     case 'keybase.1.NotifyBadges.badgeState':
       {
         const {badgeState} = action.payload.params

shared/constants/rpc/index.tsx

@@ -78,8 +78,7 @@ type Keybase1IncomingAction =
   'keybase.1.NotifyFS.FSActivity' |
   'keybase.1.NotifySession.loggedOut' |
   'keybase.1.NotifyTracking.trackingChanged' |
-  'keybase.1.NotifyUsers.userChanged' |
-  'keybase.1.reachability.reachabilityChanged'
+  'keybase.1.NotifyUsers.userChanged'
 
 type Keybase1IncomingActionMap<K extends keybase1Types.MessageKey> = {
   [P in K]: {readonly params: keybase1Types.RpcIn<P>}

shared/engine/index.platform.tsx

@@ -257,7 +257,8 @@ function createClient(
             // from a session cancel handler inside disconnectCallback must
             // not strand the UI on the disconnect banner by skipping
             // connectCallback (which synchronously clears the daemon error
-            // via startHandshake()).
+            // via startHandshake(), so nothing here may be moved behind an
+            // await).
             client.transport.reset()
             try {
               disconnectCallback()