Skip to content

Commit

Permalink
Bug 1752609 - Adds autorecovery from outdated sendtab keys. r=markh
Browse files Browse the repository at this point in the history 
Differential Revision: https://phabricator.services.mozilla.com/D139455
  • Loading branch information
Tarik Eshaq committed Apr 19, 2022
1 parent df70b3a commit b022bfa
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 91 deletions.
19 changes: 7 additions & 12 deletions services/fxaccounts/FxAccountsCommands.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,17 +372,11 @@ class SendTab {
}

async _decrypt(ciphertext) {
let sendTabKeys = await this._getPersistedSendTabKeys();
if (!sendTabKeys) {
// If we lost the user's send tab keys for any reason,
// `generateAndPersistEncryptedSendTabKeys` will regenerate the send tab keys,
// persist them, then persist an encrypted bundle of the keys.
// It should be impossible for us to hit this for new devices
// this was added to recover users who hit Bug 1752609
await this._generateAndPersistEncryptedSendTabKey();
sendTabKeys = await this._getPersistedSendTabKeys();
}
let { privateKey, publicKey, authSecret } = sendTabKeys;
let {
privateKey,
publicKey,
authSecret,
} = await this._getPersistedSendTabKeys();
publicKey = urlsafeBase64Decode(publicKey);
authSecret = urlsafeBase64Decode(authSecret);
ciphertext = new Uint8Array(urlsafeBase64Decode(ciphertext));
Expand Down Expand Up @@ -468,7 +462,8 @@ class SendTab {

async getEncryptedSendTabKeys() {
let encryptedSendTabKeys = await this._getPersistedEncryptedSendTabKey();
if (!encryptedSendTabKeys) {
const sendTabKeys = await this._getPersistedSendTabKeys();
if (!encryptedSendTabKeys || !sendTabKeys) {
log.info("Generating and persisting encrypted sendtab keys");
// `_generateAndPersistEncryptedKeys` requires the sync key
// which cannot be accessed if the login manager is locked
Expand Down
76 changes: 58 additions & 18 deletions services/fxaccounts/FxAccountsDevice.jsm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,24 +243,8 @@ class FxAccountsDevice {
log.info(
`Got new device list: ${devices.map(d => d.id).join(", ")}`
);
// Check if our push registration previously succeeded and is still
// good (although background device registration means it's possible
// we'll be fetching the device list before we've actually
// registered ourself!)
// (For a missing subscription we check for an explicit 'null' -
// both to help tests and as a safety valve - missing might mean
// "no push available" for self-hosters or similar?)
const ourDevice = devices.find(device => device.isCurrentDevice);
if (
ourDevice &&
(ourDevice.pushCallback === null || ourDevice.pushEndpointExpired)
) {
log.warn(`Our push endpoint needs resubscription`);
await this._fxai.fxaPushService.unsubscribe();
await this._registerOrUpdateDevice(currentState, accountData);
// and there's a reasonable chance there are commands waiting.
await this._fxai.commands.pollDeviceCommands();
}

await this._refreshRemoteDevice(currentState, accountData, devices);
return devices;
}
);
Expand All @@ -279,6 +263,33 @@ class FxAccountsDevice {
return this._fetchAndCacheDeviceListPromise;
}

async _refreshRemoteDevice(currentState, accountData, remoteDevices) {
// Check if our push registration previously succeeded and is still
// good (although background device registration means it's possible
// we'll be fetching the device list before we've actually
// registered ourself!)
// (For a missing subscription we check for an explicit 'null' -
// both to help tests and as a safety valve - missing might mean
// "no push available" for self-hosters or similar?)
const ourDevice = remoteDevices.find(device => device.isCurrentDevice);
if (
ourDevice &&
(ourDevice.pushCallback === null || ourDevice.pushEndpointExpired)
) {
log.warn(`Our push endpoint needs resubscription`);
await this._fxai.fxaPushService.unsubscribe();
await this._registerOrUpdateDevice(currentState, accountData);
// and there's a reasonable chance there are commands waiting.
await this._fxai.commands.pollDeviceCommands();
} else if (
ourDevice &&
(await this._checkRemoteCommandsUpdateNeeded(ourDevice.availableCommands))
) {
log.warn(`Our commands need to be updated on the server`);
await this._registerOrUpdateDevice(currentState, accountData);
}
}

async updateDeviceRegistration() {
return this._withCurrentAccountState(async currentState => {
const signedInUser = await currentState.getUserAccountData([
Expand Down Expand Up @@ -355,6 +366,35 @@ class FxAccountsDevice {
);
}

async _checkRemoteCommandsUpdateNeeded(remoteAvailableCommands) {
if (!remoteAvailableCommands) {
return true;
}
const remoteAvailableCommandsKeys = Object.keys(
remoteAvailableCommands
).sort();
const localAvailableCommands = await this._fxai.commands.availableCommands();
const localAvailableCommandsKeys = Object.keys(
localAvailableCommands
).sort();

if (
!CommonUtils.arrayEqual(
localAvailableCommandsKeys,
remoteAvailableCommandsKeys
)
) {
return true;
}

for (const key of localAvailableCommandsKeys) {
if (remoteAvailableCommands[key] !== localAvailableCommands[key]) {
return true;
}
}
return false;
}

async _updateDeviceRegistrationIfNecessary(currentState) {
let data = await currentState.getUserAccountData([
"sessionToken",
Expand Down
3 changes: 3 additions & 0 deletions services/fxaccounts/tests/xpcshell/test_accounts.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ function MockFxAccounts(credentials = null) {
observerPreloads: [],
device: {
_registerOrUpdateDevice() {},
_checkRemoteCommandsUpdateNeeded: async () => false,
},
profile: {
getProfile() {
Expand Down Expand Up @@ -245,10 +246,12 @@ async function MakeFxAccounts({ internal = {}, credentials } = {}) {
if (internal.device) {
if (!internal.device._registerOrUpdateDevice) {
internal.device._registerOrUpdateDevice = () => Promise.resolve();
internal.device._checkRemoteCommandsUpdateNeeded = async () => false;
}
} else {
internal.device = {
_registerOrUpdateDevice() {},
_checkRemoteCommandsUpdateNeeded: async () => false,
};
}
if (!internal.observerPreloads) {
Expand Down
52 changes: 52 additions & 0 deletions services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ async function MockFxAccounts(credentials, device = {}) {
},
device: {
DEVICE_REGISTRATION_VERSION,
_checkRemoteCommandsUpdateNeeded: async () => false,
},
VERIFICATION_POLL_TIMEOUT_INITIAL: 1,
});
Expand Down Expand Up @@ -817,6 +818,7 @@ add_task(async function test_refreshDeviceList() {
fxaPushService: null,
};
let device = new FxAccountsDevice(fxai);
device._checkRemoteCommandsUpdateNeeded = async () => false;

Assert.equal(
device.recentDeviceList,
Expand Down Expand Up @@ -908,6 +910,56 @@ add_task(async function test_refreshDeviceList() {
);
});

add_task(async function test_checking_remote_availableCommands_mismatch() {
const credentials = getTestUser("baz");
credentials.verified = true;
const fxa = await MockFxAccounts(credentials);
fxa.device._checkRemoteCommandsUpdateNeeded =
FxAccountsDevice.prototype._checkRemoteCommandsUpdateNeeded;
fxa.commands.availableCommands = async () => {
return {
"https://identity.mozilla.com/cmd/open-uri": "local-keys",
};
};

const ourDevice = {
isCurrentDevice: true,
availableCommands: {
"https://identity.mozilla.com/cmd/open-uri": "remote-keys",
},
};
Assert.ok(
await fxa.device._checkRemoteCommandsUpdateNeeded(
ourDevice.availableCommands
)
);
});

add_task(async function test_checking_remote_availableCommands_match() {
const credentials = getTestUser("baz");
credentials.verified = true;
const fxa = await MockFxAccounts(credentials);
fxa.device._checkRemoteCommandsUpdateNeeded =
FxAccountsDevice.prototype._checkRemoteCommandsUpdateNeeded;
fxa.commands.availableCommands = async () => {
return {
"https://identity.mozilla.com/cmd/open-uri": "local-keys",
};
};

const ourDevice = {
isCurrentDevice: true,
availableCommands: {
"https://identity.mozilla.com/cmd/open-uri": "local-keys",
},
};
Assert.ok(
!(await fxa.device._checkRemoteCommandsUpdateNeeded(
ourDevice.availableCommands
))
);
});

function expandHex(two_hex) {
// Return a 64-character hex string, encoding 32 identical bytes.
let eight_hex = two_hex + two_hex + two_hex + two_hex;
Expand Down
70 changes: 9 additions & 61 deletions services/fxaccounts/tests/xpcshell/test_commands.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -639,9 +639,9 @@ add_task(async function test_send_tab_keys_regenerated_if_lost() {
const accountState = {
data: {
// Since the device object has no
// sendTabKeys, when we _decrypt,
// we will attempt to regenerate the
// keys.
// sendTabKeys, it will recover
// when we attempt to get the
// encryptedSendTabKeys
device: {
lastCommandIndex: 10,
},
Expand All @@ -664,39 +664,12 @@ add_task(async function test_send_tab_keys_regenerated_if_lost() {
},
telemetry: new TelemetryMock(),
};

const sendTab = new SendTab(commands, fxAccounts);
sendTab._encrypt = (bytes, device) => {
return bytes;
};
let generateEncryptedKeysCalled = false;
sendTab._generateAndPersistEncryptedSendTabKey = async () => {
generateEncryptedKeysCalled = true;
};
sendTab._fxai = fxAccounts;
const tab = { title: "tab title", url: "http://example.com" };
const to = [{ id: "devid", name: "The Device" }];
const reason = "push";

await sendTab.send(to, tab);
Assert.equal(commands._invokes.length, 1);

for (let { cmd, device, payload } of commands._invokes) {
Assert.equal(cmd, COMMAND_SENDTAB);
sendTab._fxai = fxAccounts;
try {
await sendTab.handle(device.id, payload, reason);
} catch {
// The `handle` function will throw an error
// since we are not mocking the `_decrypt`
// function. This is intentional, since
// we want to capture that `_decrypt` will
// call `_generateEncryptedSendTabKeys` if
// it fails to retrieve the keys.
// Receiving a send tab is covered
// in the above test_sendtab_receive test.
}
}
await sendTab.getEncryptedSendTabKeys();
Assert.ok(generateEncryptedKeysCalled);
});

Expand All @@ -712,8 +685,10 @@ add_task(async function test_send_tab_keys_are_not_regenerated_if_not_lost() {
const accountState = {
data: {
// Since the device object has
// sendTabKeys, when we _decrypt,
// we will not try to regenerate them
// sendTabKeys, it will not try
// to regenerate them
// when we attempt to get the
// encryptedSendTabKeys
device: {
lastCommandIndex: 10,
sendTabKeys: "keys",
Expand All @@ -737,38 +712,11 @@ add_task(async function test_send_tab_keys_are_not_regenerated_if_not_lost() {
},
telemetry: new TelemetryMock(),
};

const sendTab = new SendTab(commands, fxAccounts);
sendTab._encrypt = (bytes, device) => {
return bytes;
};
let generateEncryptedKeysCalled = false;
sendTab._generateAndPersistEncryptedSendTabKey = async () => {
generateEncryptedKeysCalled = true;
};
sendTab._fxai = fxAccounts;
const tab = { title: "tab title", url: "http://example.com" };
const to = [{ id: "devid", name: "The Device" }];
const reason = "push";

await sendTab.send(to, tab);
Assert.equal(commands._invokes.length, 1);

for (let { cmd, device, payload } of commands._invokes) {
Assert.equal(cmd, COMMAND_SENDTAB);
sendTab._fxai = fxAccounts;
try {
await sendTab.handle(device.id, payload, reason);
} catch {
// The `handle` function will throw an error
// since we are not mocking the `_decrypt`
// function. This is intentional, since
// we want to capture that `_decrypt` will
// not call `_generateEncryptedSendTabKeys` if
// it succeeds to retrieve the keys.
// Receiving a send tab is covered
// in the above test_sendtab_receive test.
}
}
await sendTab.getEncryptedSendTabKeys();
Assert.ok(!generateEncryptedKeysCalled);
});
2 changes: 2 additions & 0 deletions services/fxaccounts/tests/xpcshell/test_device.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const { PREF_ACCOUNT_ROOT } = ChromeUtils.import(

_("Misc tests for FxAccounts.device");

fxAccounts.device._checkRemoteCommandsUpdateNeeded = async () => false;

add_test(function test_default_device_name() {
// Note that head_helpers overrides getDefaultLocalName - this test is
// really just to ensure the actual implementation is sane - we can't
Expand Down

0 comments on commit b022bfa

Please sign in to comment.