1550487 - OTR: Implement per-account preferences UI

Status: RESOLVED FIXED

(Chat Core :: Security: OTR, enhancement)

Description

[Kai Engert (:KaiE:)]

Based on the discussion in bug 1518185, we won't reuse most of the preferences UI from the old code (bug 1518172).

Instead, we want to implement new preferences UI, which is also per-account, so it will have to live in a new tab inside the account prefs.

Let's use this bug to track the implementation of the new OTR prefs UI.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: otr-preferences.png

Here's a screenshot of the WIP preferences panel for the chat account.
I implemented the tabs and updated the style of the panel to improve visual consistency with the other sections.
The OTR panel currently only comes with placeholders labels and an initial structure.
This should be a good boilerplate to start adding the various functionalities.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9063949 [details]
otr-preferences.png

I wonder what description we want to have for OTR. That's a hard to digest acronym...

Maybe the tab title should be "End-to-end encryption"? The "settings shouldn't be there as we're already inside settings. Likewise lower down in that tab.

There could be a brief description on top inside that tab. Something like:

One-to-one conversations can be encrypted by end-to-end encryption, so that no outside parties can listen in on the conversation. Thunderbird supports the end-to-end encryption using the OTR protocol. The conversation partner also needs to use software that supports OTR.

Comment 3

[Patrick Cloke [:clokep]]

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

I wonder what description we want to have for OTR. That's a hard to digest
acronym...

Maybe the tab title should be "End-to-end encryption"? The "settings
shouldn't be there as we're already inside settings. Likewise lower down in
that tab.

I do wonder how much of this we want to build to be "extensible", I hope that at some point in the not-so-far feature there will be other implementations of end-to-end encryption (e.g. Matrix has one built into the protocol). I'd prefer to not "leak" the underlying implementation to end-users if possible.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-05-10 15-59-32.png

I updated the UI of the preferences panel, which now includes all the strings and settings Kai will need to wire once the OTR patch lands.

I styled the fingerprint description to be selectable and resemble an input field. We could probably explore the option of adding a "Copy" button to the right of the fingerprint to facilitate the user in the selection process and prevent typos like blank last characters.

While working on this, I noticed that the accountManage.css is repeated 5 times, with all the classes duplicated. It doesn't exit a shared one with dedicated per-OS variations.
I will file a separated bug to optimize it and prevent that duplication, unless it was done on purpose.

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: otr-preferences.patch

This is the patch with the static UI.
All the functions are empty boilerplate in order to have a starting point once the main OTR patch has been merged.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9064199 [details] [diff] [review]
otr-preferences.patch

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

::: chat/locales/en-US/xmpp.properties
@@ +233,4 @@
>  # LOCALIZATION NOTE (options.*):
>  #   These are the protocol specific options shown in the account manager and
>  #   account wizard windows.
> +options.resource=Resource:

you can't change strings unless you change the localization key
but probably best to put the ":" in by code instead if you need it. You probably don't (and it looks like it's inconsistent anyway)

::: mail/components/im/content/am-im.xul
@@ +31,5 @@
> +  <groupbox>
> +    <tabbox id="imTabbox" flex="1">
> +      <tabs>
> +          <tab id="imTabGeneral" label="&account.general;"/>
> +          <tab id="imTabOTR" label="&account.encryption;"/>

nit: the tabs look pretty crammed together, so may need some styling (later)

@@ +39,5 @@
> +          <label class="header">&account.settingsTitle;</label>
> +          <hbox id="passwordBox" equalsize="always" align="baseline">
> +            <label value="&account.password;" control="server.password" flex="1"/>
> +            <textbox id="server.password" flex="1" type="password"
> +                    preftype="wstring" genericattr="true"/>

nit: alignment off

@@ +44,5 @@
> +          </hbox>
> +          <hbox id="aliasBox" equalsize="always" align="baseline">
> +            <label value="&account.alias;" control="server.alias" flex="1"/>
> +            <textbox id="server.alias" flex="1" preftype="wstring"
> +                    wsm_persist="true" genericattr="true"/>

here too

@@ +74,5 @@
> +              <label>&otr.keys.fingerprint;</label>
> +              <description id="otrFingerprint" class="fingerprintLabel">
> +                1234ASDF 1234ASDF 1234ASDF 1234ASDF
> +              </description>
> +            </hbox>

I think there was some discussion this needs to be copyable. Should likely just be an <html:input readonly="readonly">

But maybe also needs an explanation of what it is, like:

The fingerprint uniquely identifies your private key.

@@ +82,5 @@
>  
> +          <vbox>
> +            <label class="header">&otr.fingerprint.title;</label>
> +            <label>&otr.fingerprint.description;</label>
> +            <hbox flex="1" align="right">

not sure how this should look but having the view button all to the right on its own looks slightly odd

::: mail/locales/en-US/chrome/messenger/am-im.dtd
@@ +6,4 @@
>  <!ENTITY accountWindow.width           "300">
>  <!ENTITY account.general               "General">
> +<!ENTITY account.encryption            "End-to-end Encryption">
> +<!ENTITY account.otr.label             "Off the Record Protocol">

The OTR protocol

@@ +27,5 @@
> +<!ENTITY otr.settings.description      "These settings apply to all one-to-one conversations.">
> +<!ENTITY view.label                    "View">
> +<!ENTITY view.accesskey                "V">
> +<!ENTITY otr.require                   "Require encryption">
> +<!ENTITY otr.alwaysVerify              "Always nudge to verify your contact's identity">

Maybe remove the Always?

::: mail/themes/linux/mail/accountManage.css
@@ +25,4 @@
>    font-family: -moz-fixed;
>  }
>  
> +.fingerprintLabel {

these styles should be in a shared theme file (like you did for the conversionDialog)

Comment 7

[Florian Quèze [:florian]]

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

Comment on attachment 9064199 [details] [diff] [review]
otr-preferences.patch

Review of attachment 9064199 [details] [diff] [review]:

::: chat/locales/en-US/xmpp.properties
@@ +233,4 @@

LOCALIZATION NOTE (options.*):

These are the protocol specific options shown in the account manager and

account wizard windows.

+options.resource=Resource:

you can't change strings unless you change the localization key
but probably best to put the ":" in by code instead if you need it. You
probably don't (and it looks like it's inconsistent anyway)

If you need a ':', it needs to be localized. For example in French this would be: "Ressource :".

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: otr-preferences.patch

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

you can't change strings unless you change the localization key
but probably best to put the ":" in by code instead if you need it. You
probably don't (and it looks like it's inconsistent anyway)

Sounds good, I reverted those back

@@ +27,5 @@

+<!ENTITY otr.settings.description "These settings apply to all one-to-one conversations.">
+<!ENTITY view.label "View">
+<!ENTITY view.accesskey "V">
+<!ENTITY otr.require "Require encryption">
+<!ENTITY otr.alwaysVerify "Always nudge to verify your contact's identity">

Maybe remove the Always?

I think "Always" should remain. This checkbox will set the OTR to always ask for verification, even if the user you're talking to was already verified. By default, once you verify an account, it won't prompt you with the verification dialog for future connections. Maybe we could rephrase it to something like "Always ask to verify your contact's identity"?

::: mail/themes/linux/mail/accountManage.css
@@ +25,4 @@

font-family: -moz-fixed;
}

+.fingerprintLabel {

these styles should be in a shared theme file (like you did for the
conversionDialog)

I removed them since I replaced the label with the html:input field.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9065116 [details] [diff] [review]
otr-preferences.patch

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

Maybe move the OTR Settings section up under the The OTR protocol section?

What I think is also missing is some explanation that only for verified contacts you can be sure that the connection is truly secure. Thinking about this some, I think it could make sense to have that explanation in combination with the finger prints. Like:

-------------------
Verified Encryption

You can only be sure nobody is eavesdropping on the conversation when your contact has been verified by comparing public key fingerprints using an outside (out-of-band) communication channel.

Your Fingerprint: [ .......................................... ]

 [ Manager Fingerprints ]

-------------------


Sorry, didn't think if this earlier

::: mail/locales/en-US/chrome/messenger/am-im.dtd
@@ +6,5 @@
>  <!ENTITY accountWindow.width           "300">
>  <!ENTITY account.general               "General">
> +<!ENTITY account.encryption            "End-to-end Encryption">
> +<!ENTITY account.otr.label             "The OTR Protocol">
> +<!ENTITY account.otr.description       "One-to-one conversations can be encrypted by end-to-end encryption, so that no outside parties can listen in on the conversation. Thunderbird supports the end-to-end encryption using the OTR protocol. The conversation partner also needs to use software that supports OTR.">

Instead of Thunderbird this needs to be &brandShortName; 
(include <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> in the document)

The last sentence could maybe be:
To make end-to-end encryption possible, your contact also needs to use &brandShortName;  or some other software that supports the OTR protocol.

@@ +19,5 @@
>  <!ENTITY account.proxySettings.change.accessKey "C">
> +<!ENTITY account.settingsTitle         "Authentication Settings">
> +<!ENTITY account.channelTitle          "Default Channels">
> +<!ENTITY otr.keys.title                "My Private Key">
> +<!ENTITY otr.keys.fingerprint          "This is the Fingerprint that uniquely identifies your private key.">

lower case F

@@ +21,5 @@
> +<!ENTITY account.channelTitle          "Default Channels">
> +<!ENTITY otr.keys.title                "My Private Key">
> +<!ENTITY otr.keys.fingerprint          "This is the Fingerprint that uniquely identifies your private key.">
> +<!ENTITY otr.fingerprint.title         "Known Fingerprints">
> +<!ENTITY otr.fingerprint.description   "Manage the list of fingerprints you've seen.">

Instead of having this text (we don't usually do that), I think to make this more similar to other places in the UI we could have only the button and label it

Manage Fingerprints

@@ +23,5 @@
> +<!ENTITY otr.keys.fingerprint          "This is the Fingerprint that uniquely identifies your private key.">
> +<!ENTITY otr.fingerprint.title         "Known Fingerprints">
> +<!ENTITY otr.fingerprint.description   "Manage the list of fingerprints you've seen.">
> +<!ENTITY otr.settings.title            "OTR Settings">
> +<!ENTITY otr.settings.description      "These settings apply to all one-to-one conversations.">

maybe we can drop this too

@@ +26,5 @@
> +<!ENTITY otr.settings.title            "OTR Settings">
> +<!ENTITY otr.settings.description      "These settings apply to all one-to-one conversations.">
> +<!ENTITY view.label                    "View">
> +<!ENTITY view.accesskey                "V">
> +<!ENTITY otr.require                   "Require encryption">

this should probably read: Require end-to-end encryption for one-to-one conversations

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: otr-preferences.patch

Sorry, didn't think if this earlier

No problem :D
I updated everything accordingly and you're right, with those changes the tab feels tighter and lesson confusing.

A quick screenshot will follow.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-05-16 15-25-14.png

Comment 12

[Patrick Cloke [:clokep]]

I don't know if this is a good part to pick apart wording yet, but a few comments:

• Should we say "Off-the-record" on this page somewhere? It uses the OTR abbreviation everywhere.
• I believe it is usually referred to as "Off-the-Record Messaging", not "protocol". I'd like to avoid using protocol here since that has a different meaning in other context.

I think the first paragraph could be shortened quite a bit:

Daily supports end-to-end encryption of one-to-one conversations to prevent outside parties from eavesdropping in on a conversation. To make end-to-end encryption possible, your contact also needs to use Daily or some other software that supports Off-the-Record Messaging.

"Off-the-Record Messaging" here should probably link somewhere useful (a knowledge base article, wikipedia, somewhere else). (I also changed "listening" to "eavesdropping" to match the text below.)

I also have a bit of concern about the use of "one-to-one conversation" -- I don't know if this concept is "explained" anywhere, but maybe that's OK.

The verified encryption paragraph seems confusing too. Is it suggesting you verify their fingerprint? You both verify each other's? Something else.

Comment 13

[Kai Engert (:KaiE:)]

A late reply to an early comment:

(In reply to Patrick Cloke [:clokep] from comment #3)

I'd prefer to not "leak" the underlying implementation to end-users if possible.

I think we shouldn't hide it from the user. Users must be able to communicate with their friends, and tell them what software/protocol they require for encrypted chats to work. (And they cannot just tell them "install thunderbird", we aren't on mobile platforms.)

Also, should the local Thunderbird user clicks "start encrypted chat", but the other side doesn't support it, the other side will see a message that talks about the OTR protocol. It would be strange to hide that term from the local user, when we send it to the other side.

Comment 14

[Kai Engert (:KaiE:)]

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

+<!ENTITY otr.alwaysVerify "Always nudge to verify your contact's identity">

Maybe remove the Always?

I think "Always" should remain. This checkbox will set the OTR to always ask for verification, even if the user you're talking to was already verified. By default, once you verify an account, it won't prompt you with the verification dialog for future connections. Maybe we could rephrase it to something like "Always ask to verify your contact's identity"?

There's a misunderstanding here.

Once you have verified, you will not again be prompted for the same contact, as long as the contact continues to use the same private key.

The intended meaning of this "always" is: "Until I haven't verified a contact's identity, continue to remind me each time I start a new conversation with an unverified contact."

More background explanation:

After you have verified the identity, the key fingerprint of the contact is locally stored. As long as the other side continues to use the same computer, with the same files on disk, that contact will use the same key during future conversations. If it's the same, we get the same fingerprint, and can switch the status to verified.

After you have verified, going back to "unverified" only happens if your contact suddenly uses a different key (with a fingerprint that is different that we've seen previously).

Why would a contact suddenly use a different key?

• we might in fact be connected to attacker, who uses their skills/powers to open a conversation with us,
  that seems to be in the name of our usual contact.

• our usual contact might have reinstalled their system, and might have lost their previous files

• our usual contact might be using a second computer, which has different keys

With that explained, we could potentially clarify the meaning of the preference like this:

"Always remind me to verify a chat with an unverified contact"

Comment 15

[Kai Engert (:KaiE:)]

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

What I think is also missing is some explanation that only for verified
contacts you can be sure that the connection is truly secure. Thinking about
this some, I think it could make sense to have that explanation in
combination with the finger prints. Like:

---

Verified Encryption

You can only be sure nobody is eavesdropping on the conversation when your
contact has been verified by comparing public key fingerprints using an
outside (out-of-band) communication channel.

This isn't fully correct, because we offer multiple ways to verify a contact.
Comparing fingerprints is just one of them.

There are two other mechanisms for verifying the contacts identity:

• sharing a secret known by both parties
• asking a question, which only the other side can answer

These verification mechanism are easier to use than comparing a fingerprint, but they only work when both users are online.

With these alternative mechanisms, some tricky cryptographic math is used. The above information isn't transmitted and compared. Instead, on both computers, some math is used, that combines the user's respective own keys, together with the secrets, and only the results are transmitted and compared. If the calculation performed on both sides is correct, then we have mathematical proof that the other side is using the expected key. As a result, as long as the fingerprint of the key used by the other side doesn't change, we know it's still the same person (with the same key files on their system).

I'll suggest an alternative wording soon.

Comment 16

[Kai Engert (:KaiE:)]

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

@@ +21,5 @@

+<!ENTITY account.channelTitle "Default Channels">
+<!ENTITY otr.keys.title "My Private Key">
+<!ENTITY otr.keys.fingerprint "This is the Fingerprint that uniquely identifies your private key.">
+<!ENTITY otr.fingerprint.title "Known Fingerprints">
+<!ENTITY otr.fingerprint.description "Manage the list of fingerprints you've seen.">

Instead of having this text (we don't usually do that), I think to make this
more similar to other places in the UI we could have only the button and
label it

Manage Fingerprints

I think we must enable the user to understand which fingerprints are being managed here, and "manage fingerprints" seems too short.

At the very least it should say "Manage known fingerprints".

I'd prefer some additional explanation, but I think that can be given in the dialog that will open after clicking the button.

In the dialog that will open, I'd like to include text like this:

"You have previously identified the identity of the contacts that are listed below, represented by their fingerprints.

If a contact has informed you that their old key has been compromised, remove the fingerprints of that contact that you have previously verified."

Comment 17

[Kai Engert (:KaiE:)]

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

"You have previously identified the identity of the contacts that are listed below, represented by their fingerprints.

Sorry typo:

"You have previously verified the identity of the contacts that are listed below, represented by their fingerprints."

Anyway, I'll just add this suggested text when I add the popup dialog to manage the fingerprints (copying the old code).

Comment 18

[Kai Engert (:KaiE:)]

Regarding the wording for the lower part of the pref pane.

I like the heading "Verified Encryption".

I suggest to use the following text as the explanation above the "Your Fingerprint:" label and the "Manage known Fingerprints" button:

"To enable others to verify your identity in OTR chats, you may write down your own OTR fingerprint as shown below, and share it offline with your contacts.

You can view the list of contacts you have previously verified, or revoke your trust in a previous verification."

Comment 19

[Kai Engert (:KaiE:)]

After have said comment 18, I can think of a better label for the button:

"Manage previous verifications"

Comment 20

[Kai Engert (:KaiE:)]

(In reply to Patrick Cloke [:clokep] from comment #12)

• Should we say "Off-the-record" on this page somewhere? It uses the OTR abbreviation everywhere.
• I believe it is usually referred to as "Off-the-Record Messaging", not "protocol". I'd like to avoid using protocol here since that has a different meaning in other context.

I agree with Patrick that we should mention the full term "Off-The-Record Messaging".

I think the heading would be best to introduce that term, so I don't agree with Magnus's suggestion to use "The OTR Protocol" as the heading.

For the topmost heading, I'd use "Off-The-Record Messaging (OTR)", as an introduction of the acronym.
(I think we could allow localizers to translate the word "Messaging", if they want to, but only that word.)

I think the first paragraph could be shortened quite a bit:

Daily supports end-to-end encryption of one-to-one conversations to prevent outside parties from eavesdropping in on a conversation. To make end-to-end encryption possible, your contact also needs to use Daily or some other software that supports Off-the-Record Messaging.

I like Patrick's shorter version. Maybe the word "some" is unnecessary? ("... or other software that supports..."

If we use my suggestion to introduce the phrase "Off-The-Record Messaging (OTR)", then we don't need to repeat it at the end, and the end of the sentence could be shortened to "... that supports OTR.". But repeating the full phrase is fine, too, up to you.

"Off-the-Record Messaging" here should probably link somewhere useful (a knowledge base article, wikipedia, somewhere else). (I also changed "listening" to "eavesdropping" to match the text below.)

Yes, having the final word "OTR" link to wikipedia could be useful.
-> https://en.wikipedia.org/wiki/Off-the-Record_Messaging

But I don't know if we support hyperlinks in labels. If we don't, we could have a button that says "More info about OTR", which opens the browser with that URL.

I also have a bit of concern about the use of "one-to-one conversation" -- I don't know if this concept is "explained" anywhere, but maybe that's OK.

Yes, see my suggestions above how it could be changed.

Comment 21

[Kai Engert (:KaiE:)]

Attachment: otr-prefs.png

Here's a screenshot that shows the wording I suggested in the previous comments.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: patch with Kai's alternative wording

This is an updated version of aleca's patch that includes the alternative wording shown in my screenshot.

Comment 23

[Kai Engert (:KaiE:)]

I'd also suggest to reconsider the header for the tab. I'd prefer "OTR Encryption" (instead of End-to-End Encryption").

Justification:

• we already agreed that we must show the term Off-The-Record somewhere
• this tab is already full
• any future chat encryption mechanism (like matrix encryption mentioned by Patrick)
  will likely need their own separate prefs
• users should be able to understand what belongs to what,
  and heading like "OTR Encryption" and "Matrix encryption" might be
  obvious labels for that.

But I'll leave that decision to you.

Comment 24

[Magnus Melin [:mkmelin]]

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

Created attachment 9065778 [details] [diff] [review]

I like where this is heading!
For the tab header, I'd leave it as End-to-End Encryption. We can always adjust in the future if need be.

... you may write down -> you can write down

Is "offline" well understood. I liked the "outside (out-of-band) communication channel" wording better (I copied it from wikipedia).

Would also drop the sentence of "You can view the list..."

Please capitalize each word on the button label

I don't think we need to link to OTR, but if we want it should be to a knowledge-base article. Linking to other third parties is difficult, especially for l10n.

Comment 25

[Magnus Melin [:mkmelin]]

Kai, what do you think about comment 24?

Comment 26

[Kai Engert (:KaiE:)]

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

Kai, what do you think about comment 24?

ack

I'll update as you said.

Comment 27

[Jorg K (CEST = GMT+2)]

"Daily" shouldn't show up in the strings. See bug 1539900.

Comment 28

[Jorg K (CEST = GMT+2)]

Can you drop the "write it down" part? You can ring the other person on the phone or something like that.

Comment 29

[Kai Engert (:KaiE:)]

the "ring the other person" happens when you are both online, and when you have started an interactive verification inside the chat window.

Here this UI is in the preferences, where your buddy might be offline. The purpose of the sentence is to explain why we're showing the fingerprint. And one purpose is to write it down, and take it with you when you're offline.

Comment 30

[Kai Engert (:KaiE:)]

(In reply to Jorg K (GMT+2) from comment #27)

"Daily" shouldn't show up in the strings. See bug 1539900.

ack

Comment 31

[Kai Engert (:KaiE:)]

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

I like where this is heading!
For the tab header, I'd leave it as End-to-End Encryption. We can always adjust in the future if need be.

ok

... you may write down -> you can write down

ok

Is "offline" well understood. I liked the "outside (out-of-band) communication channel" wording better (I copied it from wikipedia).

Changed to:

To enable others to verify your identity in OTR chats, you can write down your own OTR fingerprint as shown below, and share it using an outside (out-of-band) communication channel with your contacts.

Would also drop the sentence of "You can view the list..."

Well ok. We can explain that in the popup dialog.

Please capitalize each word on the button label

ok

(In reply to Jorg K (GMT+2) from comment #27)

"Daily" shouldn't show up in the strings. See bug 1539900.

fixed

Comment 32

[Kai Engert (:KaiE:)]

Attachment: 1550487-v5.patch

Comment 33

[Magnus Melin [:mkmelin]]

"Daily" shouldn't show up in the strings. See bug 1539900.

That bug is for some special cases. Usually you'd still use brandShortName

Comment 34

[Jorg K (CEST = GMT+2)]

No. The brandProductName was created not to differentiate in messages. "... your contact also needs to use Daily ..." is really wrong.

Comment 35

[Magnus Melin [:mkmelin]]

How? Let's say you have a thunderbird fork, then using "... also needs to use Thunderbird" is really wrong.

Comment 36

[Kai Engert (:KaiE:)]

"To make end-to-end encryption possible, your contact also needs to use &brandShortName; or other software that supports OTR."

Jörg, if you don't like this, could you please explain why? I'm not sure I understood your comment 34.

"To enable others to verify your identity in OTR chats, you can write down your own OTR fingerprint as shown below, and share it using an outside (out-of-band) communication channel with your contacts."

How about we avoid "write down", and instead use "share"? Also I think we can simplify. New suggestion:

"To enable others to verify your identity in OTR chats, you can share your own OTR fingerprint using an outside (out-of-band) communication channel."

Comment 37

[Magnus Melin [:mkmelin]]

Sounds good to me.

Comment 38

[Kai Engert (:KaiE:)]

Attachment: 1550487-v6.patch

Comment 39

[Kai Engert (:KaiE:)]

Another decision required:

If we make the two preferences "per account" preferences, the question is, what should be the default of these prefs for old and new accounts, if the values aren't set yet?

I suggest:

• "always remind me for unverified contacts" is ON
  (people only see it once they start using OTR.)

• "always require encrypted chats" is OFF
  (if this were on by default, we'd prevent any non-OTR chats,
  so I think that's an obvious decision.)

Comment 40

[Magnus Melin [:mkmelin]]

Agreed those are good defaults.

Comment 41

[Jorg K (CEST = GMT+2)]

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

How? Let's say you have a thunderbird fork, then using "... also needs to use Thunderbird" is really wrong.

You should use brandProductName and obviously the fork will need to supply a value here.

Say you're using TB 70 Daily and your friends are using TB 68 Daily, why would it be correct to say: "... your contact also needs to use Daily ...". It should say: "... your contact also needs to use Thunderbird or other software that supports OTR."

Comment 42

[Kai Engert (:KaiE:)]

Yes, we all agree that the code needs to use the placeholder (not "Daily" for Thunderbird release etc.)

Comment 43

[Jorg K (CEST = GMT+2)]

Yes, but we don't seem to agree on which placeholder. You have brandShortName and it should be brandProductName.

The former can be Daily/Thunderbird, the second one is always Thunderbird.

Comment 44

[Magnus Melin [:mkmelin]]

I think a case can be made for both, so take your pick :)

Comment 45

[Jorg K (CEST = GMT+2)]

Please make the case why the software should tell Daily users that their friends should also be using Daily. I don't get it. The neutral string was introduced exactly for that purpose where you are using Daily but you want to refer to the general product.

Comment 46

[Magnus Melin [:mkmelin]]

Well atm it would be wrong, until Thunderbird (release) has OTR support.
"Also using Thunderbird" - but the user would not (really) be using that (but say Daily), so "Also" is also wrong, if we're picky.

Comment 47

[Kai Engert (:KaiE:)]

How about we avoid this difficult situation, and simply say:

"To make end-to-end encryption possible, your contact also needs to use software that supports OTR."

?

Comment 48

[Jorg K (CEST = GMT+2)]

See comment #41. The point is that we're not going to change the string when TB 68 goes to release.

Comment 49

[Kai Engert (:KaiE:)]

<!ENTITY account.otr.description "&brandShortName; supports end-to-end encryption of one-to-one conversations to prevent outside parties from eavesdropping in on a conversation. To make end-to-end encryption possible, your contact also needs to use software that supports OTR.">

I think this would be a good solution. The first part talks about itself. The second parts avoids a product name.

Comment 50

[Magnus Melin [:mkmelin]]

Sounds good.

Comment 51

[Kai Engert (:KaiE:)]

Taking. I'll also convert to fluent.

Comment 52

[Kai Engert (:KaiE:)]

It has been suggested to change "prevent outside parties" to "prevent third parties". This seems to be more consistent with other places, so I'll make that change, too.

In bug 1552283 we had discussed more fixes to the strings. To avoid conflicting patches, I'll merge all those changes in here.

Comment 53

[Kai Engert (:KaiE:)]

Attachment: 1550487-v7.patch (wip)

Comment 54

[Kai Engert (:KaiE:)]

Attachment: otr-prefs.png (wip, v7)

Comment 55

[Kai Engert (:KaiE:)]

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

• "always remind me for unverified contacts" is ON
  (people only see it once they start using OTR.)

• "always require encrypted chats" is OFF
  (if this were on by default, we'd prevent any non-OTR chats,
  so I think that's an obvious decision.)

It might make sense to have hidden prefs, that can allow a distributor (or corporate environment) to override these prefs.
I filed bug 1552881.

Comment 56

[Magnus Melin [:mkmelin]]

Comment on attachment 9066068 [details]
otr-prefs.png (wip, v7)

Maybe
 To make end-to-end encryption possible,.....
->
End-to-end encryption is available only when your contact also uses software that supports OTR.

Comment 57

[Kai Engert (:KaiE:)]

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

End-to-end encryption is available only when your contact also uses software
that supports OTR.

Sounds mostly ok, but I suggest two changes:

• Let's drop the word "only". In the future, we could potentially support other end-to-end encryption mechanisms.
• Let's change "contact" to "conversation partner"

Comment 58

[Kai Engert (:KaiE:)]

Attachment: prefs main (wip8)

Comment 59

[Kai Engert (:KaiE:)]

Attachment: prefs finger sub dialog (wip8)

Comment 60

[Kai Engert (:KaiE:)]

Attachment: 1550487-v8.patch (wip)

I've done several simplifications to the old prefs code, that we're importing from https://github.com/arlolra/ctypes-otr/

In the original code, the fingerprints dialog was used to manage fingerprints for all contacts.

I've changed it to manage only the list of seen fingerprints for the currently shown contact. Consequently I could remove some unnecessary columns.

The "Status" column was a bit confusing. Instead of showing labels like "verified", "private", "unused", I've changed it to only show either "active" or "inactive". We need this, because only inactive fingerprints can be removed.

I've removed the functionality to trigger a verification. I ran into some JS errors, and I believe we don't need it here. We have sufficient other places from where we can trigger a verification, and I think we should avoid yet another modal level on top of the account prefs.

Alessandro: I'm removing the "equalsize" attribute for the "your fingerprint" section, because we need much more space for the right hand side.

Comment 61

[Kai Engert (:KaiE:)]

I've changed the button label again, because "manage previous conversations" wasn't correct. The dialog that opens will shown all previously seen fingerprints, regardless if a verification was done for those or not.

I think the new label "Fingerprints Of Contacts..." is more correct, and it's also consistent with the label "Your fingerprint" directly above it.

Comment 62

[Kai Engert (:KaiE:)]

Attachment: 1550487-v9.patch

Converted to use fluent for all new strings.
Should be ready for review, I'll submit it to phabricator, too.

Comment 63

[Kai Engert (:KaiE:)]

Alex, do you want to try the patch and give feedback?

Comment 64

[Kai Engert (:KaiE:)]

Attachment: Bug 1550487

OTR: Implement per-account preferences UI

Comment 65

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-05-21 11-02-24.png

The Preferences section looks good, but I experienced some issues while trying it.
After enabling the OTR option in the config, I get this error whenever I try to access the General chat account preferences:

JavaScript error: resource:///modules/OTR.jsm, line 211: TypeError: can't pass undefined to argument 1 of char* otrl_privkey_fingerprint(struct s_OtrlUserState*, char*, char*, char*)

And as you can see from the screenshot, the panel is half empty.

Comment 66

[Kai Engert (:KaiE:)]

I cannot explain why you get a failure. I just did a fresh comm-beta build, with patch v9, and it works fine.

Maybe a conflict with some other of your local changes?
Or maybe it's broken on comm-central? I've started a fresh build as well, and will test.

Comment 67

[Alessandro Castellani [:aleca]]

I found the issue.
I needed to restart TB after enabling OTR from the config.
Is that a case the user will encounter if it enables OTR from Preferences? Or is the OTR key enabled by default?

Comment 68

[Kai Engert (:KaiE:)]

Attachment: incremental patch (on top of v9), avoid problems if "chat.otr.enable" gets changed to "true" at runtime

Thanks Alessandro! This was very helpful.
No, OTR isn't enabled by default. Yes, a user might do the same as you did.

I've changed the code to look at the chat.otr.enable pref only once, during startup of the chat system, and then we'll remember if we were successful. All other places will look at the result of the initial init attempt.

With these changes, enabling the pref at runtime shouldn't cause any change in behavior, until restart.

I'm attaching the incremental diff here, and will update the patch in the phabricator review.

As an additional improvement, with this change, the external OTR library files are loaded if the pref is enabled at startup, only.

Comment 69

[Kai Engert (:KaiE:)]

Attachment: another incremental patch, needed because of the removal of protocolName function

Comment 70

[Kai Engert (:KaiE:)]

Attachment: Bug 1550487 - OTR: Implement per-account preferences UI. r=mkmelin

Contains UI code by Alessandro Castellani <[e-mail]>

Comment 71

[Kai Engert (:KaiE:)]

Attachment: 1550487-ready.patch

patch ready to land

Comment 72

[Magnus Melin [:mkmelin]]

I have some small comments still.

Comment 73

[Kai Engert (:KaiE:)]

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

I have some small comments still.

This patch is the largest, and the base patch in a stack of patches.

Maybe we can apply the fixes for your comments on top.

Comment 74

[Magnus Melin [:mkmelin]]

Sure, do that if it's easier.

Comment 75

[Kai Engert (:KaiE:)]

I've submitted the requested changes, and are waiting for additional review feedback. The stacked patches don't conflict, so let's wait and update this one.

Comment 76

[Kai Engert (:KaiE:)]

Attachment: 1550487-ready2.patch

Comment 77

[Kai Engert (:KaiE:)]

Comment on attachment 9069755 [details] [diff] [review]
1550487-ready2.patch

needed for OTR feature

Comment 78

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0146e4f2fdaf
OTR: Implement per-account preferences UI. r=mkmelin

Comment 79

[Jorg K (CEST = GMT+2)]

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