composition security options for encrypt should only be one menu item (with checkbox) like signing

RESOLVED FIXED in Thunderbird 3.0rc1

Status

Thunderbird
Security
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Magnus Melin, Assigned: Magnus Melin)

Tracking

Trunk
Thunderbird 3.0rc1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Bv1 is fixed-thunderbird3.1a1])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
In the composition window, under Options > Security

 [v] Do not encrypt this message
 [ ] Encrypt this message
 -------------------------------
 [ ] Digitally sign this message 

As you can see, the encryptiion has two entries, signing has one, which adds the checkmark when chosen. 

S/MIME is confusing enough as it is... I don't see why there would have to be two menu items for one thing (for encryption). It should just be one, like signing.
(Assignee)

Comment 1

9 years ago
Created attachment 401700 [details] [diff] [review]
proposed fix

I actually figured out why it's implemented like it is today - should be because ns4 had a "encrypt if possible" option, so encryption could be 3-state. However, that's not implemented (bug 135636) and the UI details of a good implementation of that are probably hard to get right anyway, especially as the cases where you'd have "always require encryption" are rare for real world scenarios. So i think we could worry about that in the future, and improve what we have for now.
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #401700 - Flags: ui-review?(clarkbw)
Attachment #401700 - Flags: review?(philringnalda)
(Assignee)

Comment 2

9 years ago
Created attachment 401701 [details] [diff] [review]
screenshot of the fix

Screenshot with the patch applied.
(Assignee)

Comment 3

9 years ago
Created attachment 401702 [details]
screenshot of the fix

Screeshot of the fix (for real this time!)
Attachment #401701 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #401702 - Attachment is patch: false
Attachment #401702 - Attachment mime type: text/plain → image/png
(In reply to comment #1)
> the UI details of a good implementation of that are probably hard to get
> right anyway, especially as the cases where you'd have "always require
> encryption" are rare for real world scenarios.

"always require encryption" should be the _normal_ encrypted state unless I'm completely misunderstanding what you're saying. If encryption is worth bothering with at all then sending some (unknown number of) copies unencrypted defeats the whole point.

The "encrypt if possible" abomination was an attempt to bootstrap in a world where signing certs were rare and hard to get. We should work to solve _that_ problem, not make it easier for that state to continue.
Comment on attachment 401700 [details] [diff] [review]
proposed fix

I may be misunderstanding the bug. Simplifying the options is good, I just hope it works like the menu makes it look like it works.

>+function toggleEncryptMessage()
   ...
>+  if (gSMFields.requireEncryptMessage)
   ...
>+  else
>+  {
>+    setNoSignatureUI();
>+  }

Shouldn't that be setNoEncryptionUI() ?
(Assignee)

Comment 6

9 years ago
This bug doesn't change the functionality, i was just speculating about why it was done the way it is currently, and whether the UI should stay like it is because of that.

I don't think signing certs are easy to come by nowadays, but yeah, an "if possible" option should probably be more in along the line of "warn, and let me send unencrypted then", which would support moving to the UI proposed in this bug - as the radio group only makes sense if there is indeed a three state option.
(Assignee)

Comment 7

9 years ago
Created attachment 401722 [details] [diff] [review]
proposed fix, v2

Fix the problem dveditz noted in comment 5.
Attachment #401700 - Attachment is obsolete: true
Attachment #401722 - Flags: ui-review?(clarkbw)
Attachment #401722 - Flags: review?(philringnalda)
Attachment #401700 - Flags: ui-review?(clarkbw)
Attachment #401700 - Flags: review?(philringnalda)
(Assignee)

Updated

9 years ago
Target Milestone: --- → Thunderbird 3.0rc1
Comment on attachment 401722 [details] [diff] [review]
proposed fix, v2

Looks fine to me, as long as Bryan's okay with moving the menuitems up out of the "Ignore This Stuff" submenu they're in.

Probably be a good idea to post to m.d.l10n about the possible need to change the accesskeys, though I think you're right that it's not a certain enough need to call for changing entity names.
Attachment #401722 - Flags: review?(philringnalda) → review+
And whether or not certs are "easy" to get is only a tiny part of the problem: most of the people I send mail to either use webmail (many of them as an explicit choice so they can read it from any device anywhere with a browser) or in several cases still use AOL. "Work to solve _that_ problem" means working to eliminate legacy clients from the face of the earth, or to persuade the people using them that it's their fault that they can read mail from everyone else, but not from me, and working to make certs on implanted chips and readers for those implanted chips universally available. Otherwise, I'm simply not going to persuade my friends that my "yeah, let's go fishing up Drift Creek, meet you at the Safeway parking lot at 8" emails are so sacred and sensitive that they must change clients, and/or only be able to read them while they are at home or by installing a Firefox extension and a personal cert (and possibly Firefox) on any computer they happen to want to use to read their mail. And I'm no more likely to set the misnamed "Required", and then for 9999 out of 10000 emails I send have to click ok in a dialog, then uncheck a menuitem, then click send again, than I am to remember to check the menuitem for that 1 out of 10000, or than they are to start carrying their cert everywhere with them.

Should be an interesting race, to see whether you make that world happen before or after someone gets around to implementing jglick's 2001 spec at http://www-archive.mozilla.org/mailnews/specs/security/ for "when possible" so I will at least encrypt mail in the very few cases when I can :)
Attachment #401722 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 401722 [details] [diff] [review]
proposed fix, v2

Looks fine to me.  For any user who says "I'm going into the Options menu, be back in 5 mins", you should be prepare to wait longer.  This change might actually decrease that time by removing the "Do not encrypt this Message" option as if it were something most people actually opt'd into.
(Assignee)

Updated

9 years ago
Attachment #401722 - Flags: approval-thunderbird3?
Thank you, Magnus, for championing this UI fix, and making it happen.
Check marks make lousy radio buttons in a menu, especially when they're 
also used as ordinary non-radio buttons in the same menu.  

I'd be happy to participate in the perennial debate about the options 
for "default" behaviors with respect to encrypting outgoing emails, but 
I feel pretty strongly that bugs are wrong venue/forum for that debate. 
May I suggest mozilla.dev.security.policy?  or mozilla.dev.security?

(Shall we have a debate about the right venue for the debate? :)
Attachment #401722 - Flags: approval-thunderbird3? → approval-thunderbird3+
(Assignee)

Comment 12

9 years ago
changeset:   3863:66a98178ec65
http://hg.mozilla.org/comm-central/rev/66a98178ec65

->FIXED

Posted to m.d.l10n. 

mozilla.dev.security works for me if you want to discuss how to improve encryption behavior
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 360488
No longer blocks: 360488
Created attachment 419562 [details] [diff] [review]
(Bv1) Remove a setNoEncryptionUI() call, Add a setEncryptionUI() call
[Checkin: Comment 15]

You kept the former and removed the latter.
I haven't tested this code, but, looking at toggleSignMessage(), it looks like the opposite would be wanted.
(I noticed this while working on bug 537219.)
Attachment #419562 - Flags: review?(mkmelin+mozilla)
Attachment #419562 - Flags: review?(philringnalda)
(Assignee)

Comment 14

8 years ago
Comment on attachment 419562 [details] [diff] [review]
(Bv1) Remove a setNoEncryptionUI() call, Add a setEncryptionUI() call
[Checkin: Comment 15]

Indeed. r=mkmelin
Attachment #419562 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 419562 [details] [diff] [review]
(Bv1) Remove a setNoEncryptionUI() call, Add a setEncryptionUI() call
[Checkin: Comment 15]


http://hg.mozilla.org/comm-central/rev/379c4de191c5
Attachment #419562 - Attachment description: (Bv1) Remove a setNoEncryptionUI() call, Adda a setEncryptionUI() call → (Bv1) Remove a setNoEncryptionUI() call, Add a setEncryptionUI() call [Checkin: Comment 15]
Attachment #419562 - Flags: review?(philringnalda)
Comment on attachment 419562 [details] [diff] [review]
(Bv1) Remove a setNoEncryptionUI() call, Add a setEncryptionUI() call
[Checkin: Comment 15]

"approval-thunderbird3.0.1=?":
No risk, fix wrong call.
Attachment #419562 - Flags: approval-thunderbird3.0.1?
Comment on attachment 419562 [details] [diff] [review]
(Bv1) Remove a setNoEncryptionUI() call, Add a setEncryptionUI() call
[Checkin: Comment 15]

This is exactly why follow-on patches are not a good idea. It makes tracking extremely hard (if we take this patch on this bug, QA is either going to think we've fixed this bug again, or we're doing something strange) and it would be unclear as to what was fixed in what version from a glance.

I expect we do want this on branch, however please put this patch on a different bug, with a clear, testable description of what it is actually fixing (I've looked at it for 5 minutes and can't easily work it out yet), and then we'll consider it.
Attachment #419562 - Flags: approval-thunderbird3.0.1? → approval-thunderbird3.0.1-
Depends on: 536404
(In reply to comment #17)

> This is exactly why follow-on patches are not a good idea.

(I guess I prefer bugs related to code than to product versions :-/)

> I expect we do want this on branch, however please put this patch on a
> different bug

You moved it to bug 536404 :-)
Whiteboard: [Bv1 is fixed-thunderbird3.1a1]
You need to log in before you can comment on or make changes to this bug.