70382 - need to abstract out nsISingleSignonManager iface from nsIWallet.

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Judson Valeski]

see URL for more notes, but the gist of it is that some of the nsIWallet::SI_*
methods needs to be pulled into a public interface to support the following for
single signon:

- enumeration of elements - Get*List() strings should turn into standard
enumeration arrays so delimiter info is implicity.
- need the ability to purge an indivudual list item
- need ability to purge entire list

Comment 1

[Stephen P. Morse]

Attaching a patch to fix part of this bug.  Specifically it modifies the 
interface to use the enumeration method.  However there is much more work to be 
done on this bug but I'd rather do things one step at a time than as one big 
checkin.  So towards that goal I am presenting this first patch.

Comment 2

[Stephen P. Morse]

Attachment: patch to incorporate enumeration in the interface

Comment 3

[Stephen P. Morse]

Conrad and/or Jud, please do a review on this patch.

Comment 4

[Stephen P. Morse]

Oops, left two files out of the patch.  Attaching new patch that includes those 
files.

Comment 5

[Stephen P. Morse]

Attachment: revised patch that contains the two missing files.

Comment 6

[Conrad Carlen (not reading bugmail)]

Steve, since you did all this, should I just assign it to you?

My only complaint with this would be of naming consistency.
nsIPswdManager - Is nsIPasswordManager to long? I don't think so. When things
are abbreviated, it's a pain as you have to guess how it's abbreviated.
nsISingsign - What does the "single" part mean? If a signon is one item that the
password manager deals with, why do we need the "single" part? Isn't any item
single until you have a collection of them?

"Singsign..." is a tounge-twister (or at least a finger-twister). I've mis-typed
that so many times.
How about nsISignon and nsISignonManager?

Comment 7

[Stephen P. Morse]

Yes, you can assign it to me under one condition -- you review it first. ;-)

As for the naming, you raise some very good questions.  I would have loved to 
call it nsIPaswordManager but someone beat me to it -- shaver created such a 
module in /netwerk/base/public.

Single signon is a standard term for this feature -- it was what we originally 
called it before it got changed to password manager and it is what microsoft 
calls it.  It refers to the fact that you manually sign on to a site just a 
single time and thereafter the feature signs you on.  As for concatenating 
the name to singsign, that was a clever tongue-twister that I came up with 
several years ago when I first wrote this module and I'm damn proud of it!

Comment 8

[Judson Valeski]

we should get some naming cleared up in this checkin. I noticed that
nsIPasswordManager isn't used anywhere. Steve, maybe you should re-work that
interface to do what you need?

Comment 9

[Conrad Carlen (not reading bugmail)]

> Single signon is a standard term for this feature
You're right. I should have done a web search for "single signon" It's a very
standard term indeed. Given that, how about:
nsIPswdManager -> nsISingleSignOnManager
and
nsISingsign -> nsISingleSignOn

Comment 10

[Stephen P. Morse]

Conrad,

Good suggestions on the naming change.  I'll redo the patch right now.

Comment 11

[Stephen P. Morse]

Per private e-mail between Jud, Conrad, and myself, we have agreed to completely 
kill out the netwerk version of nsIPasswordManager.idl (it's never called) and 
use the name here.

Comment 12

[Judson Valeski]

sorry to add more mud to the water here, but rather than killing the location
(which would require a cvs attic move by leaf to retain the history), can we
just use it there and rework it, or is it really in the wrong place?

Comment 13

[Stephen P. Morse]

It's really in the wrong place.  But killing it means simply removing mention of 
it from the makefiles, not removing it from cvs.

Comment 14

[Conrad Carlen (not reading bugmail)]

Steve, if we use nsISingleSignOnManager, which I thought you liked, do we need
to worry about the existence of nsIPasswordManager?

Comment 15

[Stephen P. Morse]

Yes, we still need nsIPasswordManger.  Per the above I thought we were in 
agreement to the following namechanges:

nsIPswdManager -> nsIPasswordManager
nsISingsign -> nsISingleSignOn

Or if you prefer we could do

nsIPswdManager -> nsIPasswordManager
nsISingsign -> nsIPassword

Actually I prefer the latter

Comment 16

[Conrad Carlen (not reading bugmail)]

> nsIPswdManager -> nsIPasswordManager
> nsISingsign -> nsIPassword

I can live with that. Although, isn't nsISingleSignOn more accurate? A single
signon represents both password *and* and, right? I still prefer:

nsIPswdManager -> nsISingleSignOnManager
nsISingsign -> nsISingleSignOn

But, not to beat the issue to death - either one of those two will do, and then
we can move on to review of the actual code - whatever its name :-)

Comment 17

[Stephen P. Morse]

> Although, isn't nsISingleSignOn more accurate?

Yes, for the generic term.  But the term we use in our product is "password 
manager".

So the best solution in my opinion is my second choice above, namely:

nsIPswdManager -> nsIPasswordManager
nsISingsign -> nsIPassword

Comment 18

[Conrad Carlen (not reading bugmail)]

OK, nsIPassword and nsIPasswordManager it is.

Comment 19

[Stephen P. Morse]

Here are the revised patches to account for the name change.  Conrad, please 
review.

Comment 20

[Stephen P. Morse]

Attachment: patch with names of files changed

Comment 21

[Stephen P. Morse]

Posting yet another iteration of the patch.  Corrects a bug in memory allocation 
that I caught.  Also removes an inefficiency whereby unnecessary data was being 
transferred to the password-manager dialog.

Comment 22

[Stephen P. Morse]

Attachment: fixes memory-allocation bug

Comment 23

[Conrad Carlen (not reading bugmail)]

Excellent! This is really cleaned up API-wise The improvement from all of this
really shows in SignonViewer.js. That code is much more straightforward than it
was and it will be much easier for embeddors to build their own password UI
around this API.

Just a few things:

+PUBLIC PRInt32
+SINGSIGN_HostCount() {
+  /* force loading of the signons file */
+  si_RegisterSignonPrefCallbacks();
// Why does this happen only here and not from SINGSIGN_UserCount?
// I guess these things are only called from the enumerator but maybe
// the enumerator should make sure everything is loaded up

// From SING_SIGNEnumerate:
+  *user = userName.ToNewUnicode();
+  return NS_OK;
// You should check that allocation succeeded before returning NS_OK
// NS_OK should only be returned if the result is good! In general,
// this code doesn't do many NULL checks. For instance:
+  *host = (char *) nsMemory::Clone
+    (reject->passwordRealm, nsCRT::strlen(reject->passwordRealm) + 1);
// I'd throw in an NS_ENSURE_ARG_POINTER(host) to be safe.

+      nsIPassword *password = new nsPassword(host, user);
+      *result = password;
+      NS_ADDREF(*result);
// More NULL checks needed!
 

Comment 24

[Stephen P. Morse]

+PUBLIC PRInt32
+SINGSIGN_HostCount() {
+  /* force loading of the signons file */
+  si_RegisterSignonPrefCallbacks();
// Why does this happen only here and not from SINGSIGN_UserCount?
// I guess these things are only called from the enumerator but maybe
// the enumerator should make sure everything is loaded up

UserCount is a function of the particular host and the caller needs to supply a 
host number to the UserCount method.  So he needs to have previously made a call 
to HostCount to find out what values for the host number are valid.  Therefore 
the initialization would have already been done in HostCount and need not be 
repeated in UserCount.

I added appropriate checks for the next two items cited in the review.  
Attaching a new set of diffs that have these added checks.  Note that two more 
makefiles have changed (in netwerk/base/public) and I forgot to include that in 
the previous set of diffs.

Comment 25

[Stephen P. Morse]

Attachment: added more null checks as requested

Comment 26

[Conrad Carlen (not reading bugmail)]

With the NULL checks, except this one,

+      nsIPassword *password = new nsPassword(host, nsnull); /* second argument
is not used */
+      if (enumerator == nsnull) {
+        return NS_ERROR_OUT_OF_MEMORY;

looks good. r=ccarlen.

Comment 27

[Stephen P. Morse]

Attachment: Patch to correct careless text editing

Comment 28

[Stephen P. Morse]

Ready for super-review.  cc'ing alecf.

Comment 29

[Conrad Carlen (not reading bugmail)]

Steve, do you have the Mac stuff in hand?
You need to remove nsIPasswordManager.idl from netwerk/base/public/MANIFEST_IDL.

Comment 30

[Stephen P. Morse]

No, I don't have the mac stuff in hand.  I am well aware that there will need to 
be changes to the mac project files for the added files here and Paul Chen has 
agreed to be my mac buddy when we are ready to check in.  But you are correct, I 
can (and should) make the change to the manifest file now.  Attaching a revised 
patch.

Comment 31

[Stephen P. Morse]

Attachment: includes change to mac manifest file

Comment 32

[Alec Flett]

Issues that must be changed to get an sr=
- the IDL files should be using interCaps, not LeadingCaps (first letter lowercase)
- stuff like this:
  this.host = (host) ? host : null;
is really redundant. you can just say this.host = host; - if host has no value,
then this.host will be set to that. 
- your use of a Reject object, makes no sense - your constructor is fine, but
it's use in AddRejectToList makes no sense. Shouldn't it say:
function AddRejectToList(count, host)
{
    rejects[count] = new Reject(count, host);
}

the use of the Signon object follows this same broken pattern.

Issues that I'd like to rant about, but don't affect the sr= status:
- this whole goneSS/goneIS/etc stuff is really bad. we should not be encoding
multiple values into a string which must be re-parsed later - it's inefficient,
and difficult to maintain. Eventually, this should change to either an array of
strings, or a hashtable of string. Not to mention that "goneSS" is completely
obscure and does not describe what data is actually contained there.
- all these c-style functions (like SIGNSIGN_*) that are just called directly
from C++ methods are unnecessary and just add a level of indirection that makes
it difficult to figure out what's going on, especially when they don't quite
match the C++ method's name. Code should just exist inline in the C++ classes,
instead of calling out to these functions... yes, I realize that there is a fair
amount of legacy code, but instead of cleaning up that code, you're extending
this bad pattern.

Comment 33

[Stephen P. Morse]

Attachment: Revised patch to address the sr's comments

Comment 34

[Alec Flett]

ok, close, but you didn't fix the JavaScript to match the updated IDL, so some
of this just won't work .. such as passwordmanager.RemoveUser should be
passwordmanager.removeUser

Comment 35

[Stephen P. Morse]

Attachment: Revised patch to fix js error (my oversight)

Comment 36

[Stephen P. Morse]

Attachment: Let's try that once more

Comment 37

[Alec Flett]

sr=alecf

Comment 38

[Stephen P. Morse]

Fix checked in last night.

Comment 39

[Andrew Thompson]

Morse... should the netwerk version of the .idl file have come out of the tree?

Its existence seems to be confusing my attempts to build with CodeWarrior 6, 
which is finding the "wrong" version when trying to build wallet

Comment 40

[Stephen P. Morse]

The netwerk version of nsIPasswordManager.idl is never being used.  That's why I 
was able to re-use the same name.

If the mac is getting the wrong version, is there something more that we need to 
change for the mac?  Will simply removing the netwerk version of the file do it 
or is there some change necessary in some mac project file?  Can you do some 
experimenting and report back what it takes to get it to work correctly on the 
mac.  Thanks.

Comment 41

[Michael Dunn]

Correction: Changing QA contact for the Embed API bugs to David Epstein.

Comment 42

[David Epstein]

1. In nsIWalletService.idl, SI_RemoveUser() and SI_StorePassword() were not 
removed, even though comments say to get rid of them. Should they still be in? 
SI_get* methods were removed.
2. nsIPasswordManager.idl exists with 3 methods: addUser(), removeUser(), and 
removeReject(). Also 2 readonly attributes: enumerator & rejectEnumerator. 
Default implementations in nsPasswordManager.cpp.
3. nsIPassword.idl exists, inherits from nsISupports, with 2 readonly 
attributes: host and user. Implementations in nsPassword.cpp.

Comment 43

[Stephen P. Morse]

Yes, someday they should be removed.  But it's going to take some work.