diff --git a/packages/fxa-settings/src/lib/channels/pairing-channel.test.ts b/packages/fxa-settings/src/lib/channels/pairing-channel.test.ts index 04d678d8039..72228504f29 100644 --- a/packages/fxa-settings/src/lib/channels/pairing-channel.test.ts +++ b/packages/fxa-settings/src/lib/channels/pairing-channel.test.ts @@ -70,6 +70,19 @@ describe('PairingChannelClient', () => { expect(onConnected).toHaveBeenCalled(); }); + // A message can land in the same tick the handshake completes, so the + // listeners have to be attached by the time `connected` observers run. + it('registers channel listeners before dispatching connected', async () => { + let listenersAtConnected = 0; + client.addEventListener('connected', () => { + listenersAtConnected = mockChannel.addEventListener.mock.calls.length; + }); + + await client.open(SERVER, CHAN, VALID_KEY); + + expect(listenersAtConnected).toBe(3); + }); + it('throws for missing params', async () => { await expect(client.open('', CHAN, VALID_KEY)).rejects.toThrow( 'Invalid channel server configuration' @@ -202,7 +215,9 @@ describe('PairingChannelClient', () => { const onError = jest.fn(); client.addEventListener('error', onError); - await client.open(SERVER, CHAN, VALID_KEY); + await expect(client.open(SERVER, CHAN, VALID_KEY)).rejects.toThrow( + 'connection refused' + ); expect(client.isConnected).toBe(false); expect(onError).toHaveBeenCalled(); @@ -216,7 +231,7 @@ describe('PairingChannelClient', () => { const err = new Error('connection refused'); PairingChannel.connect.mockRejectedValueOnce(err); - await client.open(SERVER, CHAN, VALID_KEY); + await expect(client.open(SERVER, CHAN, VALID_KEY)).rejects.toThrow(err); expect(sentryMetrics.captureException).toHaveBeenCalledWith(err); }); @@ -233,7 +248,9 @@ describe('PairingChannelClient', () => { const onError = jest.fn(); client.addEventListener('error', onError); - await client.open(SERVER, CHAN, VALID_KEY); + await expect(client.open(SERVER, CHAN, VALID_KEY)).rejects.toThrow( + 'Connection to remote device closed, please try again' + ); expect(client.isConnected).toBe(false); const detail = onError.mock.calls[0][0].detail; @@ -253,11 +270,29 @@ describe('PairingChannelClient', () => { new Error('WebSocket unexpectedly closed') ); - await client.open(SERVER, CHAN, VALID_KEY); + await expect(client.open(SERVER, CHAN, VALID_KEY)).rejects.toThrow( + PairingChannelError + ); expect(sentryMetrics.captureException).not.toHaveBeenCalled(); }); + it('allows a retry after a failed open', async () => { + const { + PairingChannel, + } = require('fxa-pairing-channel/dist/FxAccountsPairingChannel.babel.umd.js'); + PairingChannel.connect.mockRejectedValueOnce( + new Error('WebSocket unexpectedly closed') + ); + await expect(client.open(SERVER, CHAN, VALID_KEY)).rejects.toThrow( + PairingChannelError + ); + + await client.open(SERVER, CHAN, VALID_KEY); + + expect(client.isConnected).toBe(true); + }); + it('rejects concurrent open during in-flight connect', async () => { const { PairingChannel, diff --git a/packages/fxa-settings/src/lib/channels/pairing-channel.ts b/packages/fxa-settings/src/lib/channels/pairing-channel.ts index 751c0ea520c..da104dbbe19 100644 --- a/packages/fxa-settings/src/lib/channels/pairing-channel.ts +++ b/packages/fxa-settings/src/lib/channels/pairing-channel.ts @@ -252,27 +252,36 @@ export class PairingChannelClient extends EventTarget { ); this.channel = channel; - this.dispatchEvent(new CustomEvent('connected')); + // Listeners go on before `connected` so a message that arrives in the + // same tick as the handshake completing is not dropped. channel.addEventListener('message', this.handleMessage); channel.addEventListener('error', this.handleError); channel.addEventListener('close', this.handleClose); + + this.dispatchEvent(new CustomEvent('connected')); } catch (err) { // A dropped socket is the channel server's normal answer for a channel it // will not serve, so it carries no signal worth an exception report. The - // flow still ends on the failure screen; the console breadcrumb keeps it - // visible in the trail of any real error that follows. - if (isChannelClosedError(err)) { + // console breadcrumb keeps it visible in the trail of any real error + // that follows. + const closed = isChannelClosedError(err); + if (closed) { console.warn('Pairing channel closed before it opened', channelId); - this.dispatchEvent( - new CustomEvent('error', { - detail: new PairingChannelError('CONNECTION_CLOSED'), - }) - ); } else { sentryMetrics.captureException(err); - this.dispatchEvent(new CustomEvent('error', { detail: err })); } + + // Report the closed socket under its own errno so a caller reading the + // error sees the same 1006 a mid-flow close produces. + const detail = closed + ? new PairingChannelError('CONNECTION_CLOSED') + : err; + this.dispatchEvent(new CustomEvent('error', { detail })); + + // Rejecting is what lets the caller drop this instance; resolving leaves + // a client that looks live and blocks every later reopen. + throw detail; } finally { this._opening = false; } diff --git a/packages/fxa-settings/src/models/integrations/pairing-supplicant-integration.test.ts b/packages/fxa-settings/src/models/integrations/pairing-supplicant-integration.test.ts index 1067e8730db..0889c4bcf34 100644 --- a/packages/fxa-settings/src/models/integrations/pairing-supplicant-integration.test.ts +++ b/packages/fxa-settings/src/models/integrations/pairing-supplicant-integration.test.ts @@ -429,6 +429,37 @@ describe('PairingSupplicantIntegration', () => { expect(integration.state).toBe(SupplicantState.Failed); expect(onError).toHaveBeenCalled(); }); + + // The channel server refuses the socket outright for a channel already + // consumed, so the Android post-OAuth reload sees the rejection rather + // than a close event on a live channel. + it('open failure while still Connecting is ignored when completion marker is set', async () => { + sessionStorage.setItem(PAIR_COMPLETE_STORAGE_PREFIX + 'c', '1'); + mockOpen.mockRejectedValueOnce( + new Error('Connection to remote device closed, please try again') + ); + const integration = createIntegration(); + const onError = jest.fn(); + const onStateChange = jest.fn(); + integration.onError = onError; + integration.onStateChange = onStateChange; + + await integration.openChannel('wss://ch.example.com', 'c', 'k'); + + expect(integration.state).toBe(SupplicantState.Connecting); + expect(onError).not.toHaveBeenCalled(); + expect(onStateChange).not.toHaveBeenCalled(); + }); + + it('allows a retry on the same channel after a failed open', async () => { + mockOpen.mockRejectedValueOnce(new Error('ws connect failed')); + const integration = createIntegration(); + + await integration.openChannel('wss://ch.example.com', 'c', 'k'); + await integration.openChannel('wss://ch.example.com', 'c', 'k'); + + expect(mockOpen).toHaveBeenCalledTimes(2); + }); }); describe('isPairing', () => { diff --git a/packages/fxa-settings/src/models/integrations/pairing-supplicant-integration.ts b/packages/fxa-settings/src/models/integrations/pairing-supplicant-integration.ts index 17e87043e9c..5183114be25 100644 --- a/packages/fxa-settings/src/models/integrations/pairing-supplicant-integration.ts +++ b/packages/fxa-settings/src/models/integrations/pairing-supplicant-integration.ts @@ -189,7 +189,10 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { private setState(state: SupplicantState): void { this._state = state; - console.info(`Emitting pairing supplicant state change event.`, {id: this._iid, state: this.state}); + console.info(`Emitting pairing supplicant state change event.`, { + id: this._iid, + state: this.state, + }); this.onStateChange?.(state); } @@ -220,7 +223,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { this.onError?.(this._error); } - hasChannel(channelId:string) { + hasChannel(channelId: string) { return !!this._channel && this._channel.channelId === channelId; } @@ -232,9 +235,8 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { channelServerUri: string, channelId: string, channelKey: string, - version:number = 1 + version: number = 1 ): Promise { - if (version === 2 && this._channel) { if (channelId === this._channel.channelId) { console.warn('Pairing channel already open!'); @@ -274,7 +276,12 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { } catch (err: unknown) { // Reset _channel so a subsequent openChannel() call can retry this._channel = null; - this.fail(err); + // A consumed channel refusing the socket is the post-OAuth reload, not a + // failure. fail() still has to run for the config and already-connected + // errors, which reject before open() dispatches any `error` event. + if (!this.isPostCompletionReconnect()) { + this.fail(err); + } } } @@ -294,8 +301,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { // In V2, the the UI flow forces the supplicant to approve first. if (this._version === 2) { this.setState(SupplicantState.WaitingForAuthority); - } - else { + } else { if (this._state === SupplicantState.WaitingForAuthorizations) { this.setState(SupplicantState.WaitingForAuthority); } else if (this._state === SupplicantState.WaitingForSupplicant) { @@ -313,11 +319,9 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { // Send OAuth request to authority if (!this._channel) { - throw new Error('Channel no longe exists!') + throw new Error('Channel no longer exists!'); } - await this._channel - .send('pair:supp:request', oauthParams); - + await this._channel.send('pair:supp:request', oauthParams); })().catch((err: unknown) => { this.fail(err); }); @@ -401,7 +405,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { ); } - checkClientInfo () { + checkClientInfo() { // no-op. The supplicant doesn't have client info. } @@ -425,7 +429,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { } if (!clientId) { - console.warn("Could not resolve a valid clientId!") + console.warn('Could not resolve a valid clientId!'); } return clientId; @@ -443,8 +447,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { }; private handleChannelError = (event: Event) => { - - console.warn('Channel error event', event) + console.warn('Channel error event', event); if (this.isPostCompletionReconnect()) { return; @@ -488,7 +491,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { if (this._version === 2) { const data = await firefox.pairOauthStart({}); if (!data) { - throw new Error('Firefox could not provide oauth params.') + throw new Error('Firefox could not provide oauth params.'); } // This following client id check is a bandaide for now, so we at least @@ -496,8 +499,8 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { // out, since there's not point in proceeding. const client_id = this.data.clientId || this.getClientId(); if (!client_id) { - console.warn('Could not determine clientId!') - throw new Error("Could not determine clientId! Cannot proceed.") + console.warn('Could not determine clientId!'); + throw new Error('Could not determine clientId! Cannot proceed.'); } const scope = [ @@ -515,7 +518,9 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { }; // Fail fast if something wasn't provided. - const missing = Object.entries(result).filter(([k,v]) => !v && k !== 'client_id').map(([k]) => k); + const missing = Object.entries(result) + .filter(([k, v]) => !v && k !== 'client_id') + .map(([k]) => k); if (missing.length) { throw new Error(`Missing required OAuth params: ${missing.join(', ')}`); } @@ -572,7 +577,7 @@ export class PairingSupplicantIntegration extends OAuthWebIntegration { } async destroy(): Promise { - console.info('Channel destroy') + console.info('Channel destroy'); this.onStateChange = null; this.onError = null;