Pop up a dialog when we get a room key request #996
Conversation
| @@ -914,6 +915,14 @@ module.exports = React.createClass({ | |||
| } | |||
| } | |||
| }); | |||
|
|
|||
| const krh = new KeyRequestHandler(cli); | |||
| cli.on("crypto.roomKeyRequest", (req) => { | |||
There was a problem hiding this comment.
API: The KeyRequestHandler is being given the entire MatrixClient, and given we never call handleKeyRequest or handleKeyRequestCancellation in any other situation, might it be better to make it the responsibility of KeyRequestHandler to attach event listeners? No strong feelings either way, just a thought.
There was a problem hiding this comment.
I thought about that. The problem is that the code ends up looking like:
new KeyRequestHandler(cli);
or possibly:
const krh = new KeyRequestHandler(cli);
krh.registerEventListenersOnTheClientYouAlreadyHave();
... neither of which make me happy in any way.
There was a problem hiding this comment.
Hmm, agreed. Let's leave it as it is.
| this.props.matrixClient.downloadKeys([userId], false).then((r) => { | ||
| if (this._unmounted) { return; } | ||
|
|
||
| const deviceInfo = r[userId][deviceId]; |
There was a problem hiding this comment.
Is it guaranteed that r[userId] will never ever ever be undefined?
|
|
||
| _onShareClicked: function() { | ||
| console.log("KeyShareDialog: User clicked 'share'"); | ||
| this.props.onFinished(true); |
There was a problem hiding this comment.
I don't know about you, but I find it really unclear that onFinished(true) means "share keys" and onFinished(false) means "don't share keys". I wonder if there utility in defining named constants for this e.g:
const SHARE_KEYS = true;
const DONT_SHARE_KEYS = false;
// ...
this.props.onFinished(SHARE_KEYS);I don't feel at all strongly about this, it's just a thought. WDYT?
There was a problem hiding this comment.
hrm. that feels a bit ott; otoh a comment at the top of KeyShareDialog explaining what the values passed to onFinshed mean does seem worthwhile.
| return ( | ||
| <BaseDialog className='mx_KeyShareRequestDialog' | ||
| onFinished={this.props.onFinished} | ||
| title='Encryption key request' |
src/KeyRequestHandler.js
Outdated
| const requestId = keyRequest.requestId; | ||
|
|
||
| if (!this._pendingKeyRequests[userId]) { | ||
| this._pendingKeyRequests[userId] = {}; |
There was a problem hiding this comment.
Whilst we enforce (somewhat) the syntax of user IDs (starting with @), AFAIK we have no restrictions whatsoever on the syntax of device IDs. This means you need to be careful when creating lookup tables like this, because {} is not empty. Consider the case of a device ID of "hasOwnProperty":
> s = {}
{}
> Boolean(s["hasOwnProperty"])
truePrefer Object.create(null) which does not inherit from the default Object prototype and is therefore a true map:
> s = Object.create(null)
{}
> Boolean(s["hasOwnProperty"])
false
| this._currentDevice = null; | ||
|
|
||
| if (r) { | ||
| for (const req of this._pendingKeyRequests[userId][deviceId]) { |
There was a problem hiding this comment.
Is your null guard on :94-:100 still valid in the finished callback or do you need to re-check in case someone has changed this._pendingKeyRequests from under you e.g. by a cancellation?
There was a problem hiding this comment.
well, all the cocking around with _currentUser and _currentDevice is to ensure that this._pendingKeyRequests can't be changed under me.
| delete this._pendingKeyRequests[userId]; | ||
| } | ||
|
|
||
| this._processNextRequest(); |
There was a problem hiding this comment.
UX: This means that dialogs may immediately pop up after you close the previous one. I don't know about you, but I find this annoying. It might be nicer to have a list of devices / users in the dialog and check boxes in the future? Not expecting anything here, just your thoughts.
... otherwise _processNextRequest will get confused
|
LGTM |
No description provided.