82377 - Add UI for private key db timeout

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Bob Lord]

There is currently no UI to control the timeout for your private key dbs.  We
should add such UI to the Certificates pref panel.  Possible wording:

$product_name will ask for your Master Password:
o The first time your certificate is needed
o Every time your certificate is needed
o After [30] minutes of inactivity

Comment 1

[Bob Lord]

Target -> 2.0.

Comment 2

[David P. Drinan]

->p2

Comment 3

[David P. Drinan]

Assigning this bug to mcgreer.

Comment 4

[David P. Drinan]

-> p1

Comment 5

[Ian McGreer]

I have this implemented, but there's problem: where to put it?  On my windows 
build, the prefs->security->certificates windows is too crowded, and this gets 
cropped (the prefs window is not resizable).  I tried putting it in 
prefs->security->passwords, another logical choice, but it didn't quite fit 
there either.  Attaching screenshots.

Comment 6

[Ian McGreer]

Attachment: what it looks like in prefs->security->certificates

Comment 7

[Ian McGreer]

Attachment: what it looks like in prefs->security->passwords

Comment 8

[Bob Lord]

Another option is to create a "Timeout" item below "Passwords".  I don't think
you're going to be able to get that new feature to fit in either of your two
proposals easily.  Even if you did and think you got it right, I suspect that
different OSes will cause that problem to come up again.  As long as the
Preferences team feels that users should not be able to resize that window and
don't like scroll bars, the only other option is to make more sub-items.

It's possible that you could put it on the "Privacy and Security" top level to
at least put *some* content there. 

Other ideas?

Comment 9

[John Unruh]

I would like to see this on the "Privacy and Security" top level, along with a 
"Change Password" button.

Comment 10

[Ian McGreer]

Attachment: what it looks like in prefs->security

Comment 11

[Ian McGreer]

Attachment: patch to put it there. thayes, can you review?

Comment 12

[Ian McGreer]

*** Bug 83167 has been marked as a duplicate of this bug. ***

Comment 13

[Ian McGreer]

Attachment: rev 2, uses text from mock-ups

Comment 14

[Ian McGreer]

More thoughts on this bug.  First, to paraphrase IRC discussion from last night:

1) Not having password prefs & change password under the "Passwords" subitem is
confusing.  Since that subitem only deals with the password manager, it should
have a more specific name.

2) We should add another subitem dealing with master passwords.  IIRC, we should
end up with two subitems: "Master Passwords", and "Web Passwords".

I went to look at PSM 1.x, and I found that the "Master Passwords" part (there
were no "Web Passwords", of course) waas handled in Security
Manager->Certificates->Passwords.  The window that popped up allowed you to both
change your password, and set the timeout preferences.

Based on that, I have a new proposal.

Put the timeout preferences in the change password dialog.  Remove the counters
for character types (another bug), and put it there.  This pref is global, so
when it is set, it applies to all tokens (avoids confusion when you have more
than one token and you're only changing the password for one of them).  Put a
change password button in both "Certificates" and "Passwords", since it applies
in both places (and a single button won't take much real estate).

Thoughts?

Comment 15

[Ian McGreer]

Terry made another point.  Why are we storing these values as prefs, when they
are stored in the slot, and PKCS#11 allows us to retrieve them?  The slot should
be more authoritative on the actual status, and each slot could manage the
settings seperately.  The timeout radiogroup would apply to each slot listed in
the change password dialog, not to all of them globally.  Then it makes even
more sense to include the timeout stuff in that dialog, not somewhere else.

Comment 16

[Ian McGreer]

Attachment: moved yet again, here's a current image. note the "web passwords" change

Comment 17

[Bob Lord]

From the user's point of view, the right design is for all tokens to timeout at
the same time.  I understand that under the covers we *can* have different
timeouts for all the tokens, but the extra complexity does not seem to be worth
it.  PKI is already too hard to deploy.

I'm pretty sure Terry was with me when we talked to Bob Relyea about this some
time ago. ;-)

Comment 18

[Ian McGreer]

Attachment: rev 3, divides "Passwords" into "Web Passwords" and "Master Passwords"

Comment 19

[Javier Delgadillo]

r=javi

Comment 20

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 21

[Stephane Saux]

PDT for PSM

Comment 22

[Stephane Saux]

PDT+ as per Steve Elmeer 6/20/2001

Comment 23

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.2

Comment 24

[Ian McGreer]

checked in

Comment 25

[John Unruh]

Reopening. The prefs are not being written to prefs.js.

Comment 26

[Ian McGreer]

John-

The preference is stored on the token.  There really shouldn't have been a 
prefs.js entry to begin with, I don't think.  It's misleading, because if you 
change your prefs.js file, you won't actually change the timeout setting (unless 
we put code in at initialization to read the value from pref.js and set it 
accordingly).

The preference is saved, correct?  If you change it, quit, and relaunch, it 
preserves the setting?

David, Terry, what do you think we should do about this?

Comment 27

[John Unruh]

Marking fixed. It works even though the prefs are not in prefs.js.

Comment 28

[Jim Dunn]

The AIX tinderbox is busted by this checkin.
in nsPK11TokenDB.cpp, the params for SetAskPasswordDefaults
are both const.  However when the nsIPK11TokenDB.h file is
generated from the idl file, the const specifier is not being
created so in essence you prototype one member function and
you are defining another and the AIX compiler does't like that.

You either need to change the function definition of
SetAskPasswordDefaults in nsPK11TokenDB.cpp from
NS_IMETHODIMP nsPK11Token::SetAskPasswordDefaults(const PRInt32 askTimes, const
PRInt32 askTimeout);
to
NS_IMETHODIMP nsPK11Token::SetAskPasswordDefaults(PRInt32 askTimes, PRInt32
askTimeout);

OR you need to force the creation of const in nsIPK11TokenDB.idl by adding [const]
void setAskPasswordDefaults([const] in long askTimes, [const] in long timeout);

See xpcom/io/nsIFile.idl line 60

If you let me know which way you would prefer, I am willing to create the
diff and checkin the fix - provided you guys give me the okeedokee

Comment 29

[Javier Delgadillo]

I say force idl to create the const.

Comment 30

[Stephane Saux]

Jim,
Please follow javi's suggestion. Thanks for doing this for us.
ssaux for PSM.

Comment 31

[Bob Lord]

I just grabbed the latest builds and looked at the UI.  I think we're almost
there, except... :-)

1. The language I proposed when I opened the bug doesn't suit all the cases
where your Master Password is needed.  The Master Password is needed for
  -SSL client-auth
  -Form Signing
  -S/MIME signing
  -S/MIME decryption
  -the Web Password feature

This last feature is the one that does not require a personal certificate, so
the phrase "The first time your personal certificate is requested" doesn't work
for me.  And I expect that more people will chose to use the encrypted web
password feature than will have personal certs (for a little while).  So for the
largest pool of users, this phrasing will be confusing.

2. The third option now says "After inactivity on an encrypted site for [20]
minutes.  This is both awkward, vague, and also does not suit all the uses for
the  feature.  The encrypted web passwords feature does not require you to visit
an SSL site.  This phrasing also does not apply to Form Signing or S/MIME.

Let me propose this language which will better fit the various ways we use the
Master Password:

&brandShortName; will ask for your Master Password:
o The first time it is needed
o Every time it is needed
o After [30] minutes of inactivity

Comment 32

[Jim Dunn]

Diff for fixing AIX tinderbox, will ask for permission to check
this into the trunk and the branch (if it has branched before I
get it into the trunk)

Index: nsIPK11TokenDB.idl
===================================================================
RCS file: /cvsroot/mozilla/security/manager/ssl/public/nsIPK11TokenDB.idl,v
retrieving revision 1.5
diff -u -r1.5 nsIPK11TokenDB.idl
--- nsIPK11TokenDB.idl  2001/06/21 13:48:30     1.5
+++ nsIPK11TokenDB.idl  2001/06/22 13:17:20
@@ -69,7 +69,7 @@
   void changePassword(in wstring oldPassword, in wstring newPassword);
   long getAskPasswordTimes();
   long getAskPasswordTimeout();
-  void setAskPasswordDefaults(in long askTimes, in long timeout);
+  void setAskPasswordDefaults([const] in long askTimes, [const] in long 
timeout);
 
   /*
    * Other attributes

Comment 33

[Ian McGreer]

Bob,

The timeout setting does not affect Password Manager.  I tried it myself.  I
actually used the word "certificates" specifically for that reason.

If it is supposed to affect Password Manager, that's another bug.

Comment 34

[Mike Kaply [:mkaply]]

Could someone please checkin the AIX bustage fix now that we have branched?

Comment 35

[Bob Lord]

I believe that we have now concluded that the timeout settings do apply to the
web password manager (when you opt to encrypt that data).  However, there is
another problem (bug 87334) which causes "Ask everytime" to not work correctly
for SDR operations.  Is that where we are now?

If so, is it then appropriate to consider the wording changes I proposed above?

Comment 36

[Ian McGreer]

I would say the wording, as is, best describes the actual functionality of the 
setting.  It is very unlikely a fix for the dependant bug will be checked in, 
considering as of right now it doesn't exist.  I would say it's unlikely people 
will use the "timeout" setting, and we could release note the fact that that 
setting *will* apparently apply to web passwords.  But most people will choose 
between "first time" and "every time" (if they choose at all), and it would be 
confusing to make them think "every time" will apply to web passwords when in 
reality it doesn't.

I would nominate what's left of this bug (the wording changes and the bug they 
depend on) for PSM 2.1.  Yes/No?

Comment 37

[Javier Delgadillo]

I've checked in the patch to fix AIX tinderbox onto the trunk (chofmann said it
was OK to check in)

Comment 38

[Jim Dunn]

Fix checked into 0.9.2 branch as well...
fyi, I reopened this bug for this AIX bustage,
in my opinion we can now close it again as fixed.
However based on Lord's comments
        >>>Additional Comments From Bob Lord 2001-06-21 22:00<<<
I am hesitant about marking this fixed.  So if someone
else who is following this, wants to mark it fixed
that is fine with me.

Comment 39

[Bob Lord]

I propose that we consider the matter of implementing the UI to be FIXED.

Please followup with other wording issues in bug 87922.

Comment 40

[John Unruh]

Verified fixed. Please followup with other wording issues in bug 87922.