Closed Bug 1552161 Opened 5 years ago Closed 5 years ago

OTR notification box: Only shown for one contact.

Categories

(Chat Core :: Security: OTR, defect)

defect
Not set
normal

Tracking

(thunderbird68 fixed, thunderbird69 fixed)

RESOLVED FIXED
Instantbird 69
Tracking Status
thunderbird68 --- fixed
thunderbird69 --- fixed

People

(Reporter: KaiE, Assigned: aleca)

References

Details

Attachments

(1 file, 1 obsolete file)

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.

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

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

Attached patch otr-notificationbox.patch (obsolete) (deleted) — Splinter Review

This should be fixed.

Attachment #9067502 - Flags: review?(kaie)
Comment on attachment 9067502 [details] [diff] [review] otr-notificationbox.patch The code works in my interactive tests.
Attachment #9067502 - Flags: review?(kaie) → review+

Asking Feedback to Magnus for this before check-in

Flags: needinfo?(mkmelin+mozilla)
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
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)

(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 on attachment 9067502 [details] [diff] [review] otr-notificationbox.patch Review of attachment 9067502 [details] [diff] [review]: ----------------------------------------------------------------- Ah you're right
Attachment #9067502 - Flags: feedback+

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

Attached patch otr-notificationbox.patch (deleted) — Splinter Review

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?

Attachment #9067502 - Attachment is obsolete: true
Flags: needinfo?(kaie)
Attachment #9067890 - Flags: review+
Attachment #9067890 - Flags: feedback+

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

Flags: needinfo?(kaie)
Attachment #9067890 - Flags: approval-comm-beta?
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2249b3ca338c
Show OTR notification box per contact. r=kaie

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 69
Attachment #9067890 - Flags: approval-comm-beta? → approval-comm-beta+
Component: General → Security: OTR
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: