From ff592f0de1d3a4c3aff2fa8ae22cadf17d247913 Mon Sep 17 00:00:00 2001 From: Jody Heavener Date: Sat, 25 Jul 2020 20:31:44 -0400 Subject: [PATCH] feat(aet): handle generateEcosystemAnonId update failure Because: - When generateEcosystemAnonId calls updateEcosystemAnonId it can fail with a 412 error, which means we tried to update the anon id but another client beat us to it. We need to handle this scenario. This commit: - When updateEcosystemAnonId fails in this method, check if it's a 412 error, and retrieve the existing anon id from the auth server to store on the account instead --- .../app/scripts/lib/auth-errors.js | 6 ++ .../app/scripts/models/account.js | 27 +++++- .../app/tests/spec/models/account.js | 84 +++++++++++++++++-- 3 files changed, 105 insertions(+), 12 deletions(-) diff --git a/packages/fxa-content-server/app/scripts/lib/auth-errors.js b/packages/fxa-content-server/app/scripts/lib/auth-errors.js index bd81f34837..1555b7c940 100644 --- a/packages/fxa-content-server/app/scripts/lib/auth-errors.js +++ b/packages/fxa-content-server/app/scripts/lib/auth-errors.js @@ -614,6 +614,12 @@ var ERRORS = { errno: 1065, message: t('The image file size is too large to be uploaded.'), }, + ECOSYSTEM_ANON_ID_UPDATE_CONFLICT: { + errno: 190, + message: t( + 'Could not update Ecosystem Anon ID because precondition failed.' + ), + }, }; export default _.extend({}, Errors, { diff --git a/packages/fxa-content-server/app/scripts/models/account.js b/packages/fxa-content-server/app/scripts/models/account.js index ec8931fc23..220edc9946 100644 --- a/packages/fxa-content-server/app/scripts/models/account.js +++ b/packages/fxa-content-server/app/scripts/models/account.js @@ -363,6 +363,7 @@ const Account = Backbone.Model.extend( * authenticationMethods, * authenticatorAssuranceLevel, * profileChangedAt, + * ecosystemAnonId, * } */ accountProfile() { @@ -458,9 +459,29 @@ const Account = Backbone.Model.extend( ); } } catch (err) { - // In the event of errors, we don't want to block any user actions, just log it - // TODO: Follow up with some better error handling - console.log('generateEcosystemAnonId failed...', err); + this.unset('ecosystemAnonId'); + + const reportException = () => { + if (this._sentryMetrics) { + this._sentryMetrics.captureException(err); + } + }; + + // If we get 412 Precondition Failed it means we + // tried to update, but another client beat us to + // it. Fetch the new anon ID and use it instead. + if (AuthErrors.is(err, 'ECOSYSTEM_ANON_ID_UPDATE_CONFLICT')) { + try { + const { ecosystemAnonId } = await this.accountProfile(); + if (ecosystemAnonId) { + this.set('ecosystemAnonId', ecosystemAnonId); + } + } catch (err) { + reportException(err); + } + } else { + reportException(err); + } } }, diff --git a/packages/fxa-content-server/app/tests/spec/models/account.js b/packages/fxa-content-server/app/tests/spec/models/account.js index 679e41a76e..3bdf003c8f 100644 --- a/packages/fxa-content-server/app/tests/spec/models/account.js +++ b/packages/fxa-content-server/app/tests/spec/models/account.js @@ -3210,22 +3210,88 @@ describe('models/account', function () { assert.isTrue(fxaClient.updateEcosystemAnonId.notCalled); }); - it('swallow any errors', async () => { - const err = new Error('failure'); - sandbox.restore(); - sandbox.stub(aet, 'generateEcosystemAnonID').rejects(err); - sandbox.spy(fxaClient, 'updateEcosystemAnonId'); + it('ignores with no options', async () => { + await account.generateEcosystemAnonId(); + assert.isTrue(fxaClient.updateEcosystemAnonId.notCalled); + }); + + it('handles 412 errors by looking up existing eco anon id', async () => { + fxaClient.updateEcosystemAnonId.restore(); + account.set({ sessionToken: SESSION_TOKEN }); + sandbox.spy(account, 'set'); + sandbox.spy(account, 'accountProfile'); + sandbox.stub(fxaClient, 'accountProfile').resolves({ + ecosystemAnonId: 'wow', + }); + sandbox.stub(fxaClient, 'sessionReauth').resolves({ + keyFetchToken: 'keyFetchToken', + unwrapBKey: 'unwrapBKey', + }); + sandbox.stub(fxaClient, 'accountKeys').resolves({ + kB: 'kB1', + }); + sandbox.stub(fxaClient, 'sessionVerificationStatus').resolves({ + sessionVerified: true, + }); + sandbox + .stub(fxaClient, 'updateEcosystemAnonId') + .rejects(AuthErrors.toError('ECOSYSTEM_ANON_ID_UPDATE_CONFLICT')); await account.generateEcosystemAnonId({ password: 'passwordzxcv', }); - assert.isTrue(fxaClient.updateEcosystemAnonId.notCalled); + sinon.assert.called(account.accountProfile); + sinon.assert.calledWith(fxaClient.accountProfile, SESSION_TOKEN); + sinon.assert.calledWith(account.set, 'ecosystemAnonId', 'wow'); }); - it('ignores with no options', async () => { - await account.generateEcosystemAnonId(); - assert.isTrue(fxaClient.updateEcosystemAnonId.notCalled); + it('reports errors thrown by accountProfile when looking up existing eco anon id because of a 412 error', async () => { + fxaClient.updateEcosystemAnonId.restore(); + account.set({ sessionToken: SESSION_TOKEN }); + sandbox.spy(account._sentryMetrics, 'captureException'); + sandbox.spy(account, 'set'); + sandbox.spy(account, 'accountProfile'); + sandbox + .stub(fxaClient, 'accountProfile') + .rejects(AuthErrors.toError('UNAUTHORIZED')); + sandbox.stub(fxaClient, 'sessionReauth').resolves({ + keyFetchToken: 'keyFetchToken', + unwrapBKey: 'unwrapBKey', + }); + sandbox.stub(fxaClient, 'accountKeys').resolves({ + kB: 'kB1', + }); + sandbox.stub(fxaClient, 'sessionVerificationStatus').resolves({ + sessionVerified: true, + }); + sandbox + .stub(fxaClient, 'updateEcosystemAnonId') + .rejects(AuthErrors.toError('ECOSYSTEM_ANON_ID_UPDATE_CONFLICT')); + + await account.generateEcosystemAnonId({ + password: 'passwordzxcv', + }); + + sinon.assert.called(account.accountProfile); + sinon.assert.calledWith(fxaClient.accountProfile, SESSION_TOKEN); + sinon.assert.called(account._sentryMetrics.captureException); + assert.isFalse(account.has('ecosystemAnonId')); + }); + + it('reports any other errors to sentry and removes the newly create anon id', async () => { + sandbox.restore(); + sandbox.spy(account._sentryMetrics, 'captureException'); + sandbox + .stub(aet, 'generateEcosystemAnonID') + .rejects(new Error('failure')); + + await account.generateEcosystemAnonId({ + password: 'passwordzxcv', + }); + + assert.isFalse(account.has('ecosystemAnonId')); + sinon.assert.called(account._sentryMetrics.captureException); }); }); });