1552161 - OTR notification box: Only shown for one contact.

Status: RESOLVED FIXED

(Chat Core :: Security: OTR, defect)

Description

[Kai Engert [:KaiE:]]

The first part of this bug is a cleanup question/request.

In https://phabricator.services.mozilla.com/D17691 regarding this code:

const gNotification = {};
XPCOMUtils.defineLazyGetter(gNotification, "notificationbox", () => {
  return new MozElements.NotificationBox(element => {
    element.setAttribute("flex", "1");
    document.getElementById("otr-notification-box").append(element);
  });
});

Florian said:

This lazy getter is strange. Why can't it be inlined at the first use?

Alex, can you please answer and potentially clean this up? (After the initial landing of bug 1518172).

Second, I found a bug related to the notification box.

• have two chat contacts
• no verifications have been done yet
  (you can reset the state up by deleting file otr.fingerprints
  in your profile directory)
• start an OTR chat with the first contact,
  and you'll get the yellow notification box
• start an OTR chat with the second contact

Actual result:

• no notification box for the second contact is shown
• when switching between the contacts,
  only a notification for the first contact is shown,
  despite for both chat sessions, the OTR button has state "unverified"

Expected result:

• notifications for both accounts should be shown

and please also verify that:

• if the notification for one gets closed,
  the notification for the second contact remains.

Please let me know if you disagree about the expected behavior.

Comment 1

[Alessandro Castellani [:aleca]]

(In reply to Kai Engert (:kaie:) from comment #0)

Florian said:

This lazy getter is strange. Why can't it be inlined at the first use?

Alex, can you please answer and potentially clean this up? (After the initial landing of bug 1518172).

This approach was first recommended by Fallen when we de-xbl'ed the notificationbox.
Mozilla is using the XPCOMUtils.defineLazyGetter and we decided to follow it as it saves up a lot of lines of code.
Every notification in our code base is using this method, so we should keep the consistency.

Second, I found a bug related to the notification box.
Expected result:

• notifications for both accounts should be shown

Yup, that's a bug, my bad. I'll take a look at it tomorrow and submit a patch.

and please also verify that:

• if the notification for one gets closed,
  the notification for the second contact remains.

Yes, that should be the expected behaviour.
I implemented the notification box to be independent and per-conversation based, but apparently some bugs slipped away.

Comment 2

[Kai Engert [:KaiE:]]

(In reply to Alessandro Castellani (:aleca) from comment #1)

This approach was first recommended by Fallen when we de-xbl'ed the notificationbox.

Thanks, I forwarded the explanation to the code review.

I'll take a look at it tomorrow and submit a patch.

Please wait until after we landed bug 1518172.

Yes, that should be the expected behaviour.

Thanks for confirming.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: otr-notificationbox.patch

This should be fixed.

Comment 4

[Kai Engert [:KaiE:]]

Comment on attachment 9067502 [details] [diff] [review]
otr-notificationbox.patch

The code works in my interactive tests.

Comment 5

[Alessandro Castellani [:aleca]]

Asking Feedback to Magnus for this before check-in

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9067502 [details] [diff] [review]
otr-notificationbox.patch

Review of attachment 9067502 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/OTRUI.jsm
@@ +505,2 @@
>      notification.setAttribute("user", context.username);
> +    notification.setAttribute("status", key);

something looks wrong here. 
Why is key being put into status? In some callers it's the verified key and in some the value is some text

Comment 7

[Alessandro Castellani [:aleca]]

(In reply to Magnus Melin [:mkmelin] from comment #6)

something looks wrong here.
Why is key being put into status? In some callers it's the verified key and
in some the value is some text

That's a string key used to style the notification box based on the current status of the encryption, as shown in the otr.css file.
#otr-notification-box notification[type="warning"][status="otr:auth-waiting"]

The different values used for that attribute are:

otr:auth-error
otr:auth-success
otr:auth-successThem
otr:auth-fail
otr:auth-waiting

I can't find where that value is the verified key, where does it happen?

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9067502 [details] [diff] [review]
otr-notificationbox.patch

Review of attachment 9067502 [details] [diff] [review]:
-----------------------------------------------------------------

Ah you're right

Comment 9

[Magnus Melin [:mkmelin]]

Constants should really not be named like "authVerify"... Uppercased would be much more clear.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: otr-notificationbox.patch

Kai, this is ready to be merged.
Do you want to wait until your patch gets reviewed and approved before mark it as checkin-needed?
Or maybe you want to make this land sooner in order to fix the notification issue?

Comment 11

[Kai Engert [:KaiE:]]

Please go ahead and land, and please also uplift to comm-beta.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2249b3ca338c
Show OTR notification box per contact. r=kaie

Comment 13

[Jorg K (CEST = GMT+2)]

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/c2c63be5e57aaff353c1023a3d0bfd8761b692e7