fillMessageKeys: allow for SessionCipher creator to specify limit #29
Conversation
|
Do we have a theory on why this seems specific to sync messages? |
There was a problem hiding this comment.
It'd be best to do this without introducing cross-repo dependencies. This change would create errors for anyone who happens to be using this library outside Signal Desktop.
Maybe we could consider simply raising this limit by default or adding it as a configurable option to SessionCipher's constructor?
src/SessionCipher.js
Outdated
| @@ -240,7 +240,10 @@ SessionCipher.prototype = { | |||
| }); | |||
| }, | |||
| fillMessageKeys: function(chain, counter) { | |||
| if (Object.keys(chain.messageKeys).length >= 1000) { | |||
| var ourNumber = textsecure.storage.user.getNumber(); | |||
There was a problem hiding this comment.
textsecure.storage is not part of this repo and we shouldn't depend on it.
There was a problem hiding this comment.
How else might a user of this library tell us who the current user is?
There was a problem hiding this comment.
I don't think this library should care about a "current user" concept, as it generally operates on a per-device level. I would just make the limit an option on SessionCipher, then the application can decide:
new SessionCipher(store, address); // limit 1000
new SessionCipher(store, address, isOurNumber ? { fillMessageKeysLimit: 5000 }); // limit 5000
new SessionCipher(store, address, isOurNumber ? { fillMessageKeysLimit: false }); // no limit
scottnonnenberg
force-pushed
the
unlimited-for-yourself
branch
from
0fdb998
to
e4144ee
Compare
scottnonnenberg
force-pushed
the
unlimited-for-yourself
branch
from
e4144ee
to
4642f87
Compare
scottnonnenberg
force-pushed
the
unlimited-for-yourself
branch
from
4642f87
to
6099145
Compare
scottnonnenberg
changed the title
|
This PR has been updated with @liliakai's recommendations. If |
If options.messageKeysLimit is provided by falsey, then we don't apply any limit at all. This can be used to set no limit for communications from your own devices. Why would you want that? People leave their laptops closed for weeks at a time and get this error, since their other devices are sending messages to it constantly: "Too many message keys for chain" And it seems to be really hard to fix once you're in this state. Sync messages no longer show up from the device that got into this state. FREEBIE
scottnonnenberg
force-pushed
the
unlimited-for-yourself
branch
from
6099145
to
b6c3093
Compare
My understanding is that this most frequently affects sync messages because if you have Right? |
There was a problem hiding this comment.
LGTM
My understanding is that if the device is offline for "a long enough time" we'll also see message loss due to the server queue being full. But I think this is a step in the right direction. It seems reasonable that sync queues/chains could get special treatment.
|
Exactly. You communicate far, far more with your own devices than anyone else. |
Quite a few changes to reflect new behavior around identity keys rolled
out in the Signal apps in June 2017:
- We always trust a new identity key when receiving messages
- We have a few requirements for trust when sending messages:
1) it can't have changed very recently
2) if the previous identity key had been verified by the user, the
user needs to explicitly approve a new identity key
Expose ability to delete all existing sessions - for proper reset (#35)
Allow caller to provide fillMessageKeys limit - allowing customization
of the maxiumum number of one-sided messages in a conversation (#29)
Dev:
- A number of changes to make the branch build in CI, add badge, etc.
Note: we appear to have some unreliability during our Firefox 34 builds
on Sauce. A little less than half the time the test fails with an out of
memory error.
People leave their laptops closed for weeks at a time, and get this error, since their other devices are sending messages to it constantly:
"Too many message keys for chain"
And it seems to be really hard to fix once you're in this state. Sync messages no longer show up from the device that got into this state.