OTR: When receiving verify request, start with notification
Categories
(Chat Core :: Security: OTR, enhancement, P1)
Tracking
(Not tracked)
People
(Reporter: KaiE, Assigned: aleca)
References
Details
Attachments
(1 file, 5 obsolete files)
26.33 KB,
patch
|
mkmelin
:
review+
KaiE
:
review+
|
Details | Diff | Splinter Review |
With the initial OTR implementation, if an incoming request for verification is received, we currently immediately open up the dialog that asks for details.
This has two problems:
-
the user might be completely surprised and have no idea what's
going on -
the user can go around that popup, and click the main thunderbird
window, and not notice the other dialog again.
I think this should be changed.
Potential improvement:
Don't bring up the dialog immediately.
Instead, show a notification bar, inform the user, offer a button, and require the user to click that button. Then proceed with the incoming request.
However, we're already showing a yellow notification, whenever we start an unverified session.
As a result, the user might miss this incoming request.
How could we distinguish the frequent notification
"... has not been verified yet... you should verify"
from
incoming verification request
?
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
I'm starting to work on this one.
The first approach I thought to try is to temporarily hide the "you should verify" yellow notification, and add the "incoming verification request" with a different colour.
With this method tho, we don't have any visual ques for the user that a contact required a verification.
We should implement an "unread counter" in the conversation to flag the user that an action is required, but then that could be confusing since it's not really an unread message.
We could also add the verification request directly inline the conversation, like a message, and style it make it stick until the user interacts with it.
This could justify the unread notification and remove the issue of having multiple notifications interfering with each other.
Assignee | ||
Comment 2•5 years ago
|
||
This was more complicated than I expected.
I had to change the structure of the chatconversation
element in order to properly use notifications.
The current setup doesn't work at all, with the notification breaking the UI of the conversation area.
I guess we never caught this error as these notifications are basically never used.
I'm doing a couple of things I'm not super happy about it.
- I'm retrieving the current conversation with this:
let conv = this.globalDoc.getElementsByTagName("chat-conversation");
this.noficationbox = conv[0].msgNotificationBar;
But ti doesn't seem right as the conversation should be part of the OTRUI, but I can't seem to find where it's store/registered.
- I had to edit the
msgNotificationBar()
method of thechant-conversation
element in order to be able to grab it since it's a getter and an error was being throws when doingreturn this.msgNotificationBar = newNotificationBox;
This worries me as may other notificationboxes are handled the same way, but we never encountered issues or reported bugs related to those sections.
Did I make a massive mistake when de-xbl'ing the notificationBox and no one ever noticed because those section are barely used?
With this patch, the current workflow is the following:
- A user requests the verification of a buddy's identity.
- A notification bar appears above the chat conversation window.
- The user can click the button to verify or dismiss the notification.
- Once the user sends the verification, or cancels the dialog, the notification gets removed.
- If the verification is successful, the OTR notification on the right panel gets updated and the "Verify" button appears if the other user wasn't verified.
Assignee | ||
Updated•5 years ago
|
Comment 3•5 years ago
|
||
Comment on attachment 9083806 [details] [diff] [review] 1551590-otr-verify-notification.patch Review of attachment 9083806 [details] [diff] [review]: ----------------------------------------------------------------- Seems I'm not able to verify at all with this patch. (Dunno why.) This is the case at least when one side is verified, and you go to verify the other. That verification req is never shown (or sent or processed, whatever) on the other side. Perhaps not this bug, but I think the dialogs to do the verification are also pretty clunky the way they present themselves. It would be much nicer and clearer if we used a door hanger from the relevant contact instead (see GlobalPopupNotifications.jsm) ::: chat/content/otr/otrUI.ftl @@ +12,5 @@ > > auth-error = An error occurred while verifying your contact's identity. > auth-success = Verifying your contact's identity completed successfully. > auth-successThem = Your contact has successfully verified your identity. You may want to verify their identity as well by asking your own question. > +auth-fail = Failed to { auth-label }. This seems wrong. Why parametrise this, and it would put capitalization in the middle of the sentence. @@ +77,3 @@ > error-title = Error > success-title = End to End Encryption > +successThem-title = { auth-label } Likely this shouldn't be parametrized to the same thing either. Whey do we have two keys if they will be the same value? ::: chat/modules/OTRUI.jsm @@ +474,5 @@ > + }, > + }]; > + > + let priority = this.globalBox.PRIORITY_WARNING_MEDIUM; > + let conv = this.globalDoc.getElementsByTagName("chat-conversation"); this.visibleConv?
Assignee | ||
Comment 4•5 years ago
|
||
This needs a bit more work to fix all the quirks I'm stumbling upon.
if I use the this.visibleConv
the notification risks to show up for the wrong contact, or not show up at all if the conversation is not open.
I'm considering the fact to have a notification area outside the chat conversation element in order to make notifications independent and show/hide them based on the currently selected conversation, something similar to what we're doing for the OTR notifications.
I'm asking some feedback from Kaie to see how can I improve this and if maybe there's a better solution I haven't considered.
Reporter | ||
Comment 5•5 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #4)
if I use the
this.visibleConv
the notification risks to show up for the wrong contact, or not show up at all if the conversation is not open.
I tested with coyim and pidgin-otr. If there's no conversation yet, they don't offer to start a verification.
I think you can assume that requests for verification will be received only for active conversations.
It's true that the user might not yet have opened/clicked that conversation.
I'm considering the fact to have a notification area outside the chat conversation element in order to make notifications independent and show/hide them based on the currently selected conversation, something similar to what we're doing for the OTR notifications.
I haven't yet tried your patch.
Maybe my original suggestion in this bug wasn't ideal.
Here's a different idea, but I don't know how easy that's possible, and I don't know if you're already doing that in your patch.
A verification request is a chat event. We could consider it as a similar event to having a message received.
Currently, when a new chat message is received, I get a quick popup with that message. If the chat window isn't selected, the chat toolbar icon gets a counter icon.
Could we do the same for incoming verification requests?
The popup could say "received an identity verification request".
Reporter | ||
Comment 6•5 years ago
|
||
I've started a comm-central build and will give feedback once I have it. I see your patch applies to comm-central, only.
Assignee | ||
Comment 7•5 years ago
|
||
I think going with the chat message appraoch might be the easiest and better solution.
I'll leave this WIP patch here and start a separated one.
Assignee | ||
Comment 8•5 years ago
|
||
I'm having a bit of trouble identifying the correct workflow for when the OTR writes a message in chat.
Could you give me a quick overview?
Sorry to bother you with this.
Reporter | ||
Comment 9•5 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #8)
I'm having a bit of trouble identifying the correct workflow for when the OTR writes a message in chat.
Why do you need that? For catching the event, you need to identify where we listen to incoming verification requests. For producing the popup notification, you need to identify how the chat code reacts to any incoming message, for a window that isn't foreground. That's independent of OTR.
Reporter | ||
Comment 10•5 years ago
|
||
I see you already identify OTRUI.askAuth as the place where we handle the incoming request.
It seems notifications are produced in mail/components/im/modules/chatNotifications.jsm when it observes "new-directed-incoming-message".
I think we need a new mechanism that tells the chatNotifications.jsm to popup a notification, even if we aren't receiving a display message.
You probably don't want the OTR code to send out a regular message into the chat system, as it would show up in the text log, right?
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 11•4 years ago
|
||
This is a very rough proof of concept in order to gather feedback and discuss the proper approach.
This is what I'd like to implement, and what I think it might be a decent solution:
Case 1 - The user doesn't have the Chat Tab open.
- A contact starts an OTR chat and sends a verification request.
- The user gets pinged with the chat message sounds and the
1
counter on the Chat button and the conversation richlistitem. - Upon selecting the conversation, the user will see a top notification bar with the verification request from the user.
Case 2 - The user has the Chat Tab open but no open conversations
- A contact starts an OTR chat and sends a verification request.
- The user gets pinged with the chat message sounds and the
1
counter only on the conversation richlistitem. - Upon selecting the conversation, the user will see a top notification bar with the verification request from the user.
Case 3 - The user has the Chat Tab open and a conversation with his contact is open
- Th contact starts an OTR chat and sends a verification request.
- A top notification bar with the verification request from the contact will appear.
In this patch I only implemented the chat sound by calling the "new-directed-incoming-message"
event, and the top notification bar.
I created a custom observer called "new-otr-verification-request"
, which I will use in order to not write any message in the conversation body and update the count to include opened notifications.
Another thing that needs to be changed is preventing the Chat Tab to automatically open and gain the focus when a new OTR request gets triggered. This can be very annoying and potentially disruptive for a user currently using another section of TB.
Overall, this is not optimal as the Chat UI is very clunky and is full of strange paradigms.
I think this approach might be good enough for a first release, but a potential substantial rewrite of the entire chat UI needs to happen.
Let me know what you think and if I'm going towards the right direction.
Assignee | ||
Comment 12•4 years ago
|
||
A little bit of progress in this patch as I fixed the issue of not being able to print the notification if the user doesn't have the conversation opened.
Everything else I talked and asked in comment 11 is still valid.
Reporter | ||
Comment 13•4 years ago
|
||
Alessandro, I like this approach.
Case 3 already works for me. I get the new notification.
For case 1 and case 2, I get a sound, but I don't see any visual indication outside the conversation yet.
Maybe the reason is the following error seen in the error console:
TypeError: can't access property "isChat", conv is undefined chatNotifications.jsm:112:13
Assignee | ||
Comment 14•4 years ago
|
||
(In reply to Kai Engert (:KaiE:) from comment #13)
Alessandro, I like this approach.
Great!
For case 1 and case 2, I get a sound, but I don't see any visual indication outside the conversation yet.
Yeah, I haven't completed this part.
The problem is that the current system expects a message, and without a message the whole notification system with the message counter doesn't work.
I'm implementing a dedicated observer for this scenario in order to leverage the message counter to include unread notifications, and not only messages.
Assignee | ||
Comment 15•4 years ago
|
||
This should be ready for a full review.
Comment 16•4 years ago
|
||
Comment on attachment 9132122 [details] [diff] [review] 1551590-otr-verify-notification.patch Review of attachment 9132122 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/public/imIConversationsService.idl @@ +18,5 @@ > // Note: this will not be logged. > void systemMessage(in AUTF8String aMessage, [optional] in boolean aIsError); > > + // Trigger a notification counter update to the conversation. > + void notificationMessage(in AUTF8String aMessage); This should be named something different. It's not really clear to me what it's doing. Please also document when it should be or is called. @@ +31,5 @@ > readonly attribute unsigned long unreadTargetedMessageCount; > // Number of unread incoming messages (both targeted and untargeted > // messages are counted). > readonly attribute unsigned long unreadIncomingMessageCount; > + // Number of unread OTR notifications. Please document what an OTR notification is ::: chat/content/otr/otrUI.ftl @@ +72,5 @@ > # Variables: > # $name (String) - the screen name of a chat contact person > afterauth-unverified = The identity of { $name } has not been verified. > > +verify-title = Verify { auth-label }'s identity I don't think you should do this. For many names it's not correct even in English. Maybe Verify the identity of { auth-label } ... and is that correct without the $?
Assignee | ||
Comment 17•4 years ago
|
||
Thanks for the review and the string update requests.
Let me know if this makes more sense.
Reporter | ||
Comment 18•4 years ago
|
||
I've tested the patch, and it seems to work well.
Reporter | ||
Comment 19•4 years ago
|
||
Comment on attachment 9132336 [details] [diff] [review] 1551590-otr-verify-notification.patch no complaints from me, good work.
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 20•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/09f75ac5b059
OTR: improve notifications when a chat starts with a verification request. r=kaie
Reporter | ||
Comment 21•4 years ago
|
||
can we uplift to beta 75?
Reporter | ||
Comment 22•4 years ago
|
||
well, although it might be nice, we should probably bake on c-c first. But might be a candidate for uplift.
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 23•4 years ago
|
||
This doesn't seem to work anymore on trunk.
Kai, can you confirm?
Assignee | ||
Comment 24•4 years ago
|
||
Nevermind, I didn't notice that my chat buddy was already verified, that's why I wasn't getting any notification.
Sorry for the ping.
Comment 25•4 years ago
|
||
Comment on attachment 9132336 [details] [diff] [review] 1551590-otr-verify-notification.patch Uplift was missed.
Description
•