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.

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

Use the device change notifications interface #348

Merged
merged 7 commits into from
Feb 3, 2017

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Feb 2, 2017

Listen to the notifications from /sync to tell us when a user's devices are out
of date. We're leaving the m.new_device stuff in place for now, for
compatibility with old homeservers/clients.

Builds on #347.

Fixes (together with the server-side support) element-hq/element-web#2305.

Only permit one query per user at a time.
@richvdh
Copy link
Member Author

richvdh commented Feb 2, 2017

Should also fix element-hq/element-web#2562

@richvdh
Copy link
Member Author

richvdh commented Feb 2, 2017

And element-hq/element-web#2564

@ara4n ara4n assigned ara4n and unassigned dbkr Feb 2, 2017
* @param {MatrixError} data.err The matrix error if <code>state=ERROR</code>.
*
* @param {String} data.oldSyncToken The 'since' token passed to /sync.
Copy link
Member

Choose a reason for hiding this comment

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

is this new, ooi? or just fixing missing doc?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's new.

@@ -318,6 +318,10 @@ export default class DeviceList {
this._sessionStore.storeEndToEndDevicesForUser(
userId, storage,
);

if (token) {
this._sessionStore.storeEndToEndDeviceSyncToken(token);
Copy link
Member

Choose a reason for hiding this comment

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

hm. ooi, is there any reason why this would differ from the general /sync token? presumably this will collide with @kegsay's incremental sync stuff?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's slightly different - the point is to store a token representing the time when we last successfully updated the device lists (which may be some time before the last successful sync) - so that when we resume, we can ask for the users which have been updated since then.

When @kegsay's stuff lands, we'll need to watch out that we still do the /changes dance, even though we won't be doing an initial sync - because we don't have a record of which users were out-of-date at the point of exit. (Or we could keep a localstorage/indexeddb record of which users are out of sync.)

@@ -48,12 +48,16 @@ const DeviceList = require('./DeviceList').default;
* @param {string} userId The user ID for the local user
*
* @param {string} deviceId The identifier for this device.
*
* @param {Object} clientStore the MatrixClient data store.
Copy link
Member

Choose a reason for hiding this comment

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

yay!

@@ -106,12 +110,6 @@ function Crypto(baseApis, eventEmitter, sessionStore, userId, deviceId) {
function _registerEventHandlers(crypto, eventEmitter) {
eventEmitter.on("sync", function(syncState, oldState, data) {
try {
if (syncState == "PREPARED") {
// XXX ugh. we're assuming the eventEmitter is a MatrixClient.
Copy link
Member

Choose a reason for hiding this comment

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

yay^2!


const rooms = this._getE2eRooms();
for (const r of rooms) {
const members = r.getJoinedMembers();
Copy link
Member

Choose a reason for hiding this comment

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

Do we care at all about invited users?
How does E2E work with peeking, if at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

To-date, we have currently glossed over those points. Invited users don't get the keys. Peeking users certainly don't.

So yeah, we need to care about it, but there are bigger problems with them.

@ara4n
Copy link
Member

ara4n commented Feb 2, 2017

LGTM.

Update some comments, and s/flushNewDeviceRequests/refreshOutdatedDeviceLists/.
When we get a notification from /sync that a user has updated their device
list, mark the list outdated, and then fire off a device query.
... so that we can, in future, use it when restarting the client.
Pass a store into the Crypto object so that it doesn't need to make assumptions
about the EventEmitter, and use the new metadata on sync events to distinguish
between initialsyncs and normal syncs
On initialsync, call the /keys/changes api to see which users have updated
their devices. (On failure, invalidate all of them).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants