Closed Bug 1556989 Opened 5 years ago Closed 5 years ago

OTR manage fingerprints: disable "remove all" button if list is empty

Categories

(Chat Core :: Security: OTR, defect)

Product:
Chat Core
Component:
Security: OTR
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Instantbird 76

People

(Reporter: KaiE, Assigned: aleca)

References

Details

Attachments

(1 file, 5 obsolete files)

1556989-otr-remove-fingerprints.diff
(deleted), patch
aleca
: review+
Details | Diff | Splinter Review

OTR manage fingerprints: disable "remove all" button is list is empty

Summary: OTR manage fingerprints: disable "remove all" button is list is empty → OTR manage fingerprints: disable "remove all" button if list is empty
Component: General → Security: OTR
Attached patch 1556989-otr-remove-fingerprints.diff (obsolete) (deleted) — — Splinter Review

I started working on this but there are some problems with this dialog.
When launching it I get these errors:

JavaScript error: chrome://chat/content/otr-finger.js, line 1: SyntaxError: redeclaration of var Localization
JavaScript error: chrome://chat/content/otr-finger.xhtml, line 1: ReferenceError: otrFinger is not defined

It seems we're experiencing a leak from the OTR.jsm file with some variables redeclaration.
I tried wrapping the code around a {} block but that doesn't seem to solve the problem.

Assignee: nobody → alessandro
Status: NEW → ASSIGNED
Attachment #9131242 - Flags: review?(kaie)

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

When launching it I get these errors:

JavaScript error: chrome://chat/content/otr-finger.js, line 1: SyntaxError: redeclaration of var Localization
JavaScript error: chrome://chat/content/otr-finger.xhtml, line 1: ReferenceError: otrFinger is not defined

It seems we're experiencing a leak from the OTR.jsm file with some variables redeclaration.
I tried wrapping the code around a {} block but that doesn't seem to solve the problem.

Not sure it's a "leak". OTR.jsm defines its exports, so local variables shouldn't have an impact on other files.

I don't fully understand why we get that error. However, I remember that Magnus complained in the past that I used "const" for an import. An in fact, if we change that "var", the error goes away.

With that fixed, I get additional errors. The assignment to the account argument fails, because it's differently structured. I fixed that, too.

Finally, I got an error from Fluent (ftl) that some attributes from finger.ftl are loaded synchronously, and others are asynchrounosly. That's probably because the xhtml refers to it and loads async, while in the .js file we request sync loading. I don't know if that's a big problem, but it seemed to be fixed easily, by moving the strings we want to load synchronously into a separate ftl file.

Attached patch 1556989-errors.patch (obsolete) (deleted) — — Splinter Review
Attachment #9131559 - Flags: review?(alessandro)
Comment on attachment 9131242 [details] [diff] [review] 1556989-otr-remove-fingerprints.diff > onload() { > fingerTree = document.getElementById("fingerTree"); > gFingers = OTR.knownFingerprints(account); > fingerTreeView.rowCount = gFingers.length; > fingerTree.view = fingerTreeView; >+ document.getElementById("remove-all").disabled = !gFingers.length; > }, You also need to disable it after you've removed entries, and all entries are gone. >+ <script src="chrome://global/content/globalOverlay.js"/> >+ <script src="chrome://global/content/editMenuOverlay.js"/> Why do you need those?
Comment on attachment 9131559 [details] [diff] [review] 1556989-errors.patch Review of attachment 9131559 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/content/otr-finger.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > const { Services } = ChromeUtils.import("resource:///modules/imServices.jsm"); > const { OTR } = ChromeUtils.import("resource:///modules/OTR.jsm"); Better to have these (too) as var When it's not a module, you very easily get redeclaration problems when using let and const for top level things. @@ +8,4 @@ > "resource://gre/modules/Localization.jsm" > ); > > +const syncL10n = new Localization(["messenger/otr/finger-sync.ftl"], true); maybe we can change this to just var l10n

Thanks both for the reviews, I'm updating the patch.

Now I'm dealing with another error, which is:

JavaScript error: chrome://chat/content/otr-finger.js, line 13: 
TypeError: can't access property Symbol.iterator, window.arguments.account is undefined

I think this is happening because I don't have any fingerprints stored.
Also, is there a reason why some objects, variables, and methods are declared outside the otrFinger object?

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

I think this is happening because I don't have any fingerprints stored.

Not true, as doing gFingers = OTR.knownFingerprints(window.arguments.account); works and shows all my fingerprints.

Attached patch 1556989-otr-remove-fingerprints.diff (obsolete) (deleted) — — Splinter Review

All right, this solves everything and the button properly updates.

Attachment #9131242 - Attachment is obsolete: true
Attachment #9131559 - Attachment is obsolete: true
Attachment #9131242 - Flags: review?(kaie)
Attachment #9131559 - Flags: review?(alessandro)
Attachment #9131574 - Flags: review?(mkmelin+mozilla)
Attachment #9131574 - Flags: review?(kaie)

I get "ReferenceError: reference to undefined property "account"" in line 66.
Change it to: window.arguments[0].account

Note there's a global object "document.l10n" which works asynchrounously. I had named our local variable syncL10n to make it obvious for the reader that we're using the non-standard approach of synchronous loading (because we use the "true" parameter when constructing the object). IMHO it might be better to keep the existing name, for self-documentation purposes, but up to you.

Other than that, patch looks good to me.

Attached patch 1556989-otr-remove-fingerprints.diff (obsolete) (deleted) — — Splinter Review

Mh, strange, I wasn't getting that error even on an empty list.
Anyway, updated and it works perfectly.

Regarding l10n vs syncL10n, I don't have a strong opinion on that, so I'm good with what Magnus decides.

Attachment #9131574 - Attachment is obsolete: true
Attachment #9131574 - Flags: review?(mkmelin+mozilla)
Attachment #9131574 - Flags: review?(kaie)
Attachment #9131593 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9131593 [details] [diff] [review] 1556989-otr-remove-fingerprints.diff (In reply to Alessandro Castellani (:aleca) from comment #10) > Mh, strange, I wasn't getting that error even on an empty list. > Anyway, updated and it works perfectly. It was a yellow line in the error console, maybe I have turned on additional messages that you don't get by default?
Attachment #9131593 - Flags: review+
Comment on attachment 9131593 [details] [diff] [review] 1556989-otr-remove-fingerprints.diff Review of attachment 9131593 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/content/otr-finger.js @@ +79,5 @@ > + for (let i = 0; i < count; i++) { > + selection.getRangeAt(i, min, max); > + for (let k = min.value; k <= max.value; k++) { > + if (k != -1) { > + selections[selections.length] = k; selections.push(k) would be more clear @@ +95,5 @@ > > remove() { > fingerTreeView.selection.selectEventsSuppressed = true; > // mark fingers for removal > + this.getSelections(fingerTree).forEach(function(sel) { while here, can we change this to for (let sel of this.getSelections(fingerTree)) ::: chat/content/otr/finger-sync.ftl @@ +4,5 @@ > + > +finger-yes = Verified > +finger-no = Unverified > + > +finger-subset-title = Removing fingerprints Remove Fingerprints @@ +5,5 @@ > +finger-yes = Verified > +finger-no = Unverified > + > +finger-subset-title = Removing fingerprints > +finger-subset-message = At least one fingerprint couldn't be removed, because you have an active conversation with the respective conversation partner. maybe "because the fingerprint is used in currently active conversation" @@ +7,5 @@ > + > +finger-subset-title = Removing fingerprints > +finger-subset-message = At least one fingerprint couldn't be removed, because you have an active conversation with the respective conversation partner. > + > +finger-remove-all-title = Removing all fingerprints Remove All Fingerprints @@ +8,5 @@ > +finger-subset-title = Removing fingerprints > +finger-subset-message = At least one fingerprint couldn't be removed, because you have an active conversation with the respective conversation partner. > + > +finger-remove-all-title = Removing all fingerprints > +finger-remove-all-message = Are you sure you want to remove all previously seen fingerprints? As a consequence, all previous OTR identity verifications will be lost. This operation cannot be undone. Would remove "As a consequence, " and also the "This operation cannot be undone"
Attachment #9131593 - Flags: review?(mkmelin+mozilla) → review+
Attached patch 1556989-otr-remove-fingerprints.diff (obsolete) (deleted) — — Splinter Review

Thanks both for the review.
I updated the message to because the fingerprint is currently used in an active conversation as it's grammatically more correct.

I launched a try run, even if I think we currently don't have any OTR test going on: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0a6dec02054e8d37e11c630f6785320c0c074319

Attachment #9131593 - Attachment is obsolete: true
Attachment #9132319 - Flags: review+

maybe "because the fingerprint is used in currently active conversation"

Sorry for my late response, but I disagree with the suggested text. Attempts for simplification shouldn't result in incorrect text.

We aren't using fingerprints. We're using keys, and the fingerprint is just an identifier that we use to label keys.

The original text is correct. If you don't like the original text, we need to try to come up with a shorter text that's still precise.

What about because the corresponding key is currently used in an active conversation?

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

What about because the corresponding key is currently used in an active conversation?

Yes that's good.
r+

I think this example shows that our existing wording isn't easy to understand, and that it should be reviewed. Maybe we should use the term "fingerprint" less. I'll file a separate bug for that.

String updated.

Attachment #9132319 - Attachment is obsolete: true
Attachment #9132350 - Flags: review+
Keywords: checkin-needed-tb
Target Milestone: --- → Instandbird 76

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/368f0e33482a
Disable 'Remove All' button in OTR manage fingerprints list if the list is empty. r=kaie, r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: