Closed Bug 1551590 Opened 1 year ago Closed 5 months ago

OTR: When receiving verify request, start with notification

Categories

(Chat Core :: Security: OTR, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 76

People

(Reporter: KaiE, Assigned: aleca)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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
?

Type: defect → enhancement

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.

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.

  1. 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.

  1. I had to edit the msgNotificationBar() method of the chant-conversation element in order to be able to grab it since it's a getter and an error was being throws when doing return 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.
Attachment #9083806 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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?
Attachment #9083806 - Flags: review?(mkmelin+mozilla) → review-

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.

Attachment #9083806 - Attachment is obsolete: true
Attachment #9084837 - Flags: feedback?(kaie)

(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".

I've started a comm-central build and will give feedback once I have it. I see your patch applies to comm-central, only.

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.

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.

Flags: needinfo?(kaie)

(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.

Flags: needinfo?(kaie)

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?

Attachment #9084837 - Flags: feedback?(kaie)
Component: General → Security: OTR
Priority: -- → P1

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.

Attachment #9084837 - Attachment is obsolete: true
Attachment #9131354 - Flags: feedback?(mkmelin+mozilla)
Attachment #9131354 - Flags: feedback?(kaie)

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.

Attachment #9131354 - Attachment is obsolete: true
Attachment #9131354 - Flags: feedback?(mkmelin+mozilla)
Attachment #9131354 - Flags: feedback?(kaie)
Attachment #9131641 - Flags: feedback?(mkmelin+mozilla)
Attachment #9131641 - Flags: feedback?(kaie)

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

(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.

This should be ready for a full review.

Attachment #9131641 - Attachment is obsolete: true
Attachment #9131641 - Flags: feedback?(mkmelin+mozilla)
Attachment #9131641 - Flags: feedback?(kaie)
Attachment #9132122 - Flags: review?(mkmelin+mozilla)
Attachment #9132122 - Flags: review?(kaie)
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 $?
Attachment #9132122 - Flags: review?(mkmelin+mozilla)

Thanks for the review and the string update requests.
Let me know if this makes more sense.

Attachment #9132122 - Attachment is obsolete: true
Attachment #9132122 - Flags: review?(kaie)
Attachment #9132336 - Flags: review?(mkmelin+mozilla)
Attachment #9132336 - Flags: review?(kaie)

I've tested the patch, and it seems to work well.

Comment on attachment 9132336 [details] [diff] [review]
1551590-otr-verify-notification.patch

no complaints from me, good work.
Attachment #9132336 - Flags: review?(kaie) → review+
Attachment #9132336 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Instandbird 76

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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED

can we uplift to beta 75?

well, although it might be nice, we should probably bake on c-c first. But might be a candidate for uplift.

Attachment #9132336 - Flags: approval-comm-beta?
Attachment #9132336 - Flags: approval-comm-beta? → approval-comm-beta+

This doesn't seem to work anymore on trunk.
Kai, can you confirm?

Flags: needinfo?(kaie)

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.

Flags: needinfo?(kaie)
Comment on attachment 9132336 [details] [diff] [review]
1551590-otr-verify-notification.patch

Uplift was missed.
Attachment #9132336 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.