Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move Device Tracking Data to Crypto Store #594

Merged
merged 22 commits into from
Jan 29, 2018
Merged

Move Device Tracking Data to Crypto Store #594

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
83e4aa2
Initial attempt at device tracking -> indexeddb
dbkr Jan 11, 2018
29d44f8
delay saving so we can batch multiple operations
dbkr Jan 12, 2018
60052f5
Add localstorage & memory device data stores
dbkr Jan 12, 2018
2cdb010
Fix lint
dbkr Jan 12, 2018
6c1087e
Migrate from session store
dbkr Jan 12, 2018
a94f3c7
Fix migration
dbkr Jan 12, 2018
36d7d33
Null fix & lint
dbkr Jan 12, 2018
110f43a
Make DeviceList test pass
dbkr Jan 15, 2018
27d28b8
More test fixing
dbkr Jan 15, 2018
0174c56
Make tests pass, finally.
dbkr Jan 16, 2018
5a0997d
lint
dbkr Jan 17, 2018
585ea14
more lint
dbkr Jan 17, 2018
5bab864
Fix device tracking with initial /sync
dbkr Jan 17, 2018
461acbc
More test fixing
dbkr Jan 17, 2018
fb29da4
Update the catchingup flag
dbkr Jan 17, 2018
ea6974f
Return null if device data isn't set
dbkr Jan 17, 2018
8b649ce
All the copyrights
dbkr Jan 18, 2018
074cfb7
Add more comments
dbkr Jan 18, 2018
4fbf4f1
Hopefully address PR feedback
dbkr Jan 22, 2018
8c2645c
Comment typo
dbkr Jan 24, 2018
470bd23
Add new devices as we get them
dbkr Jan 25, 2018
c3bede5
Fux lying comment
dbkr Jan 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions spec/TestClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*
Copyright 2016 OpenMarket Ltd
Copyright 2017 Vector Creations Ltd
Copyright 2018 New Vector Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -25,6 +26,7 @@ import testUtils from './test-utils';
import MockHttpBackend from 'matrix-mock-request';
import expect from 'expect';
import Promise from 'bluebird';
import LocalStorageCryptoStore from '../lib/crypto/store/localStorage-crypto-store';

/**
* Wrapper for a MockStorageApi, MockHttpBackend and MatrixClient
Expand All @@ -46,14 +48,19 @@ export default function TestClient(
if (sessionStoreBackend === undefined) {
sessionStoreBackend = new testUtils.MockStorageApi();
}
this.storage = new sdk.WebStorageSessionStore(sessionStoreBackend);
const sessionStore = new sdk.WebStorageSessionStore(sessionStoreBackend);

// expose this so the tests can get to it
this.cryptoStore = new LocalStorageCryptoStore(sessionStoreBackend);

this.httpBackend = new MockHttpBackend();
this.client = sdk.createClient({
baseUrl: "http://" + userId + ".test.server",
userId: userId,
accessToken: accessToken,
deviceId: deviceId,
sessionStore: this.storage,
sessionStore: sessionStore,
cryptoStore: this.cryptoStore,
request: this.httpBackend.requestFn,
});

Expand Down
147 changes: 89 additions & 58 deletions spec/integ/devicelist-integ-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,20 @@
/*
Copyright 2017 Vector Creations Ltd
Copyright 2018 New Vector Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import expect from 'expect';
import Promise from 'bluebird';

Expand Down Expand Up @@ -151,9 +168,12 @@ describe("DeviceList management:", function() {
aliceTestClient.httpBackend.flush('/keys/query', 1).then(
() => aliceTestClient.httpBackend.flush('/send/', 1),
),
aliceTestClient.client._crypto._deviceList.saveIfDirty(),
]);
}).then(() => {
expect(aliceTestClient.storage.getEndToEndDeviceSyncToken()).toEqual(1);
aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => {
expect(data.syncToken).toEqual(1);
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the test changes are the same pattern: add the explicit save calls and wait on them, and change the fetch from the store to the new callback based API.


// invalidate bob's and chris's device lists in separate syncs
aliceTestClient.httpBackend.when('GET', '/sync').respond(200, {
Expand Down Expand Up @@ -185,19 +205,21 @@ describe("DeviceList management:", function() {
return aliceTestClient.httpBackend.flush('/keys/query', 1);
}).then((flushed) => {
expect(flushed).toEqual(0);
const bobStat = aliceTestClient.storage
.getEndToEndDeviceTrackingStatus()['@bob:xyz'];
if (bobStat != 1 && bobStat != 2) {
throw new Error('Unexpected status for bob: wanted 1 or 2, got ' +
bobStat);
}

const chrisStat = aliceTestClient.storage
.getEndToEndDeviceTrackingStatus()['@chris:abc'];
if (chrisStat != 1 && chrisStat != 2) {
throw new Error('Unexpected status for chris: wanted 1 or 2, got ' +
chrisStat);
}
return aliceTestClient.client._crypto._deviceList.saveIfDirty();
}).then(() => {
aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => {
const bobStat = data.trackingStatus['@bob:xyz'];
if (bobStat != 1 && bobStat != 2) {
throw new Error('Unexpected status for bob: wanted 1 or 2, got ' +
bobStat);
}
const chrisStat = data.trackingStatus['@chris:abc'];
if (chrisStat != 1 && chrisStat != 2) {
throw new Error(
'Unexpected status for chris: wanted 1 or 2, got ' + chrisStat,
);
}
});

// now add an expectation for a query for bob's devices, and let
// it complete.
Expand All @@ -216,15 +238,18 @@ describe("DeviceList management:", function() {
// wait for the client to stop processing the response
return aliceTestClient.client.downloadKeys(['@bob:xyz']);
}).then(() => {
const bobStat = aliceTestClient.storage
.getEndToEndDeviceTrackingStatus()['@bob:xyz'];
expect(bobStat).toEqual(3);
const chrisStat = aliceTestClient.storage
.getEndToEndDeviceTrackingStatus()['@chris:abc'];
if (chrisStat != 1 && chrisStat != 2) {
throw new Error('Unexpected status for chris: wanted 1 or 2, got ' +
bobStat);
}
return aliceTestClient.client._crypto._deviceList.saveIfDirty();
}).then(() => {
aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => {
const bobStat = data.trackingStatus['@bob:xyz'];
expect(bobStat).toEqual(3);
const chrisStat = data.trackingStatus['@chris:abc'];
if (chrisStat != 1 && chrisStat != 2) {
throw new Error(
'Unexpected status for chris: wanted 1 or 2, got ' + bobStat,
);
}
});

// now let the query for chris's devices complete.
return aliceTestClient.httpBackend.flush('/keys/query', 1);
Expand All @@ -234,16 +259,18 @@ describe("DeviceList management:", function() {
// wait for the client to stop processing the response
return aliceTestClient.client.downloadKeys(['@chris:abc']);
}).then(() => {
const bobStat = aliceTestClient.storage
.getEndToEndDeviceTrackingStatus()['@bob:xyz'];
const chrisStat = aliceTestClient.storage
.getEndToEndDeviceTrackingStatus()['@chris:abc'];

expect(bobStat).toEqual(3);
expect(chrisStat).toEqual(3);
expect(aliceTestClient.storage.getEndToEndDeviceSyncToken()).toEqual(3);
return aliceTestClient.client._crypto._deviceList.saveIfDirty();
}).then(() => {
aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => {
const bobStat = data.trackingStatus['@bob:xyz'];
const chrisStat = data.trackingStatus['@bob:xyz'];

expect(bobStat).toEqual(3);
expect(chrisStat).toEqual(3);
expect(data.syncToken).toEqual(3);
});
});
});
}).timeout(3000);

// https://github.com/vector-im/riot-web/issues/4983
describe("Alice should know she has stale device lists", () => {
Expand All @@ -262,13 +289,15 @@ describe("DeviceList management:", function() {
},
);
await aliceTestClient.httpBackend.flush('/keys/query', 1);
await aliceTestClient.client._crypto._deviceList.saveIfDirty();

const bobStat = aliceTestClient.storage
.getEndToEndDeviceTrackingStatus()['@bob:xyz'];
aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => {
const bobStat = data.trackingStatus['@bob:xyz'];

expect(bobStat).toBeGreaterThan(
0, "Alice should be tracking bob's device list",
);
expect(bobStat).toBeGreaterThan(
0, "Alice should be tracking bob's device list",
);
});
});

it("when Bob leaves", async function() {
Expand Down Expand Up @@ -297,12 +326,15 @@ describe("DeviceList management:", function() {


await aliceTestClient.flushSync();
await aliceTestClient.client._crypto._deviceList.saveIfDirty();

const bobStat = aliceTestClient.storage
.getEndToEndDeviceTrackingStatus()['@bob:xyz'];
expect(bobStat).toEqual(
0, "Alice should have marked bob's device list as untracked",
);
aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => {
const bobStat = data.trackingStatus['@bob:xyz'];

expect(bobStat).toEqual(
0, "Alice should have marked bob's device list as untracked",
);
});
});

it("when Alice leaves", async function() {
Expand Down Expand Up @@ -330,12 +362,15 @@ describe("DeviceList management:", function() {
);

await aliceTestClient.flushSync();
await aliceTestClient.client._crypto._deviceList.saveIfDirty();

const bobStat = aliceTestClient.storage
.getEndToEndDeviceTrackingStatus()['@bob:xyz'];
expect(bobStat).toEqual(
0, "Alice should have marked bob's device list as untracked",
);
aliceTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => {
const bobStat = data.trackingStatus['@bob:xyz'];

expect(bobStat).toEqual(
0, "Alice should have marked bob's device list as untracked",
);
});
});

it("when Bob leaves whilst Alice is offline", async function() {
Expand All @@ -344,23 +379,19 @@ describe("DeviceList management:", function() {
const anotherTestClient = await createTestClient();

try {
anotherTestClient.httpBackend.when('GET', '/keys/changes').respond(
200, {
changed: [],
left: ['@bob:xyz'],
},
);
await anotherTestClient.start();
anotherTestClient.httpBackend.when('GET', '/sync').respond(
200, getSyncResponse([]));
await anotherTestClient.flushSync();
await anotherTestClient.client._crypto._deviceList.saveIfDirty();

const bobStat = anotherTestClient.storage
.getEndToEndDeviceTrackingStatus()['@bob:xyz'];
anotherTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => {
const bobStat = data.trackingStatus['@bob:xyz'];

expect(bobStat).toEqual(
0, "Alice should have marked bob's device list as untracked",
);
expect(bobStat).toEqual(
0, "Alice should have marked bob's device list as untracked",
);
});
} finally {
anotherTestClient.stop();
}
Expand Down
15 changes: 10 additions & 5 deletions spec/integ/matrix-client-crypto.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*
Copyright 2016 OpenMarket Ltd
Copyright 2017 Vector Creations Ltd
Copyright 2018 New Vector Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -154,11 +155,15 @@ function aliDownloadsKeys() {

// check that the localStorage is updated as we expect (not sure this is
// an integration test, but meh)
return Promise.all([p1, p2]).then(function() {
const devices = aliTestClient.storage.getEndToEndDevicesForUser(bobUserId);
expect(devices[bobDeviceId].keys).toEqual(bobTestClient.deviceKeys.keys);
expect(devices[bobDeviceId].verified).
toBe(0); // DeviceVerification.UNVERIFIED
return Promise.all([p1, p2]).then(() => {
return aliTestClient.client._crypto._deviceList.saveIfDirty();
}).then(() => {
aliTestClient.cryptoStore.getEndToEndDeviceData(null, (data) => {
const devices = data.devices[bobUserId];
expect(devices[bobDeviceId].keys).toEqual(bobTestClient.deviceKeys.keys);
expect(devices[bobDeviceId].verified).
toBe(0); // DeviceVerification.UNVERIFIED
});
});
}

Expand Down
43 changes: 32 additions & 11 deletions spec/unit/crypto/DeviceList.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,24 @@
/*
Copyright 2017 Vector Creations Ltd
Copyright 2018 New Vector Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import DeviceList from '../../../lib/crypto/DeviceList';
import MockStorageApi from '../../MockStorageApi';
import WebStorageSessionStore from '../../../lib/store/session/webstorage';
import MemoryCryptoStore from '../../../lib/crypto/store/memory-crypto-store.js';
import testUtils from '../../test-utils';
import utils from '../../../lib/utils';

Expand Down Expand Up @@ -40,13 +58,15 @@ const signedDeviceList = {
describe('DeviceList', function() {
let downloadSpy;
let sessionStore;
let cryptoStore;

beforeEach(function() {
testUtils.beforeEach(this); // eslint-disable-line no-invalid-this

downloadSpy = expect.createSpy();
const mockStorage = new MockStorageApi();
sessionStore = new WebStorageSessionStore(mockStorage);
cryptoStore = new MemoryCryptoStore();
});

function createTestDeviceList() {
Expand All @@ -56,7 +76,7 @@ describe('DeviceList', function() {
const mockOlm = {
verifySignature: function(key, message, signature) {},
};
return new DeviceList(baseApis, sessionStore, mockOlm);
return new DeviceList(baseApis, cryptoStore, sessionStore, mockOlm);
}

it("should successfully download and store device keys", function() {
Expand All @@ -72,7 +92,7 @@ describe('DeviceList', function() {
queryDefer1.resolve(utils.deepCopy(signedDeviceList));

return prom1.then(() => {
const storedKeys = sessionStore.getEndToEndDevicesForUser('@test1:sw1v.org');
const storedKeys = dl.getRawStoredDevicesForUser('@test1:sw1v.org');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can pull the stored key straight out of the DeviceList now rather than inspecting the backing storage, which I think makes it more of a unit test.

expect(Object.keys(storedKeys)).toEqual(['HGKAWHRVJQ']);
});
});
Expand All @@ -97,14 +117,15 @@ describe('DeviceList', function() {
dl.invalidateUserDeviceList('@test1:sw1v.org');
dl.refreshOutdatedDeviceLists();

// the first request completes
queryDefer1.resolve({
device_keys: {
'@test1:sw1v.org': {},
},
});

return prom1.then(() => {
dl.saveIfDirty().then(() => {
// the first request completes
queryDefer1.resolve({
device_keys: {
'@test1:sw1v.org': {},
},
});
return prom1;
}).then(() => {
// uh-oh; user restarts before second request completes. The new instance
// should know we never got a complete device list.
console.log("Creating new devicelist to simulate app reload");
Expand All @@ -121,7 +142,7 @@ describe('DeviceList', function() {
// allow promise chain to complete
return prom3;
}).then(() => {
const storedKeys = sessionStore.getEndToEndDevicesForUser('@test1:sw1v.org');
const storedKeys = dl.getRawStoredDevicesForUser('@test1:sw1v.org');
expect(Object.keys(storedKeys)).toEqual(['HGKAWHRVJQ']);
});
});
Expand Down
Loading