80094 - implement device manager in PSM2

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Ian McGreer]

track implementation of device manager

Comment 1

[Ian McGreer]

Attachment: rev 1 of patch to implement device manager

Comment 2

[Ian McGreer]

javi and/or terry, can you review?

Comment 3

[Ian McGreer]

review comments from thayes:

device_manager.js:

* You don't need contractid values for nsPKCS11Slot, nsPKCS11Module or 
nsPK11Token.  In general you only need these if you register the type 
with the component manager.  That applies to nsPK11TokenDB and 
nsPKCS11Module, but not the others.

What does the "don't ask" comment mean?  If it's not worth explaining 
the problem then delete the comment.

--fixed

| slotnames[slotnames.length] = x |  might be rewritten as | 
slotnames.append(x) | .  At least *I* like it better!

--didn't work for me (not a JS expert)

* I'd appreciate at least a one-line comment for each function (to tell 
me what it does).

--done

in AddModule(),  does | item.setAttribute("persist", "open"); |  
actually do anything?  Does it actually remember the state the next time 
you open the window.  If not, we should just remove it.

--done 

In getSelectedSlot(),  a comment on the | 
items[0].parentNode.parentNode.firstChild.firstChild | would be nice.  
Or you could call a function ( "getModuleCellForSlotCell" ) that helps 
me know what's happening.

--done

In enableButtons() and other places, you have a "here's the workaround" 
comment.  What problem are you correcting?  Or is this just stray text?

In showSlotInfo() you use "devinfo_status" and "tok_status" strings 
multiple times.  You might just want to declare a const for these.

* In doLogin(), you have English phrases in the code.  You need to use 
the properties like in doLogout().

--done

In doUnload(), what does "to be implemented by pkcs11 object" mean?

--waiting on javi's PKCS#11 JS object, which he will check in soon

doLoadDevice() doesn't seem to do anything useful (yet?).

--same as above

device_manager.xul:

* Please remove the xmlns:html definition.  We don't use XHTML anymore 
in our XUL files (in general).

Can we remove the | debug="0" | attributes?

--both done

The onselect handler calls | enableButtons() |  I'd prefer to see this 
call "doSelect" or something similar, and then have it perform the 
actual work.

load_device.xul:

* Remove the xmlns:html attribute.

Remove | debug="0" |

--done

* After you close this window, how does the device manager window 
redisplay (or repaint) the information.  It would seem that a new module 
should be available for display unless the user cancelled.

--will take care of when implementation is in

nsIPK11TokenDB.idl:

* Why are there slot-related attributes added to the token interface.  
In particular, shouldn't slotDesc, slotManID, slotHWVersion, 
slotFWVersion and (possibly) slotStatus go in the new nsIPKCS11Slot 
interface instead?

--done

nsIPKCS11Slot.idl:

* You don't need the contractid values for nsPKCS11Slot and nsPKCS11Module.

--done

nsPKCS11Slot.cpp:

*  In nsPKCS11Module::FindSlotByName:
    | asciiname = NS_CONST_CAST(char*, 
NS_ConvertUCS2toUTF8(aName).get());  |  This is a memory management 
error.  The value returned from get() will disappear when this statement 
completes.  You can't use it in the following statements.

* Is PORT_* exported from NSS?  It might be better to use the Mozilla 
string comparison routines (nsCRT??)

*  FindModuleByName has the same memory management problem as 
FindSlotByName.

--all 3 done

I'm more concerned about the items with "*" on them.  I'd like to see 
the answer to where the slot attributes end up, but other than that, if 
you fix the "*" items I'll give you the r=thayes.

Terry

--I will attach a revised patch soon (maybe tomorrow). 

Comment 4

[Ian McGreer]

Attachment: rev 2, based on terry's comments

Comment 5

[Ian McGreer]

adding blizzard, requested sr=

Comment 6

[Christopher Blizzard (:blizzard)]

in LoadModules() this code:

+  var modules = secmoddb.listModules();
+  var done = false;
+  try {
+    modules.isDone();
+  } catch (e) { done = true; }
+  while (!done) {
+    var module = modules.currentItem().QueryInterface(nsIPKCS11Module);
+    if (module) {
+      var slotnames = [];
+      var slots = module.listSlots();
+      var slots_done = false;
+      try {
+        slots.isDone();
+      } catch (e) { slots_done = true; }
+      while (!slots_done) {
+        var slot = slots.currentItem().QueryInterface(nsIPKCS11Slot);
+        // in the ongoing discussion of whether slot names or token names
+        // are to be shown, I've gone with token names because NSS will
+        // prefer lookup by token name.  However, the token may not be
+        // present, so maybe slot names should be listed, while token names
+        // are "remembered" for lookup?
+        slotnames[slotnames.length] = slot.name;
+        try {
+          slots.next();
+        } catch (e) { slots_done = true; }
+      }
+      AddModule(module.name, slotnames);
+    }
+    try {
+      modules.next();
+    } catch (e) { done = true; }
+  }

seems to really abuse iterators.  For example, can't you just do
while(slots.next()) instead of using a variable to test the end of the
condition?  Or do you do that because slots.next() throw an exception which I
also notice that you test for.  It seems odd that they would do that.  Same with
modules.next(), etc.

+        nsPKCS11Slot.cpp                \

That's off when compared to the other files to build listed in the makefile.

Instead of using this in ConvertPK11String:

+ while (actualLen >= 0 && 
+         (pk11str[actualLen] == ' ' || pk11str[actualLen] == '\0'))
+    actualLen--;

it might be easier to use nsString.Trim(" ", PR_FALSE, PR_TRUE);  You might even
be able to eliminate the ConvertPK11String call altogether.

Fix that and you have an sr=blizzard

Comment 7

[Terry Hayes]

ian: it looks like nsIEnumerator is deprecated.  Can we switch to
nsISimpleEnumerator instead?  It fixes the (really bogus) interface.

Comment 8

[Christopher Blizzard (:blizzard)]

in LoadModules() this code:

+ var modules = secmoddb.listModules();
+ var done = false;
+ try {
+ modules.isDone();
+ } catch (e) { done = true; }
+ while (!done) {
+ var module = modules.currentItem().QueryInterface(nsIPKCS11Module);
+ if (module) {
+ var slotnames = [];
+ var slots = module.listSlots();
+ var slots_done = false;
+ try {
+ slots.isDone();
+ } catch (e) { slots_done = true; }
+ while (!slots_done) {
+ var slot = slots.currentItem().QueryInterface(nsIPKCS11Slot);
+ // in the ongoing discussion of whether slot names or token names
+ // are to be shown, I've gone with token names because NSS will
+ // prefer lookup by token name. However, the token may not be
+ // present, so maybe slot names should be listed, while token names
+ // are "remembered" for lookup?
+ slotnames[slotnames.length] = slot.name;
+ try {
+ slots.next();
+ } catch (e) { slots_done = true; }
+ }
+ AddModule(module.name, slotnames);
+ }
+ try {
+ modules.next();
+ } catch (e) { done = true; }
+ }

seems to really abuse iterators. For example, can't you just do
while(slots.next()) instead of using a variable to test the end of the
condition? Or do you do that because slots.next() throw an exception which I
also notice that you test for. It seems odd that they would do that. Same with
modules.next(), etc.

+ nsPKCS11Slot.cpp \

That's off when compared to the other files to build listed in the makefile.

Instead of using this in ConvertPK11String:

+ while (actualLen >= 0 &&
+ (pk11str[actualLen] == ' ' || pk11str[actualLen] == '\0'))
+ actualLen--;

it might be easier to use nsString.Trim(" ", PR_FALSE, PR_TRUE); You might even
be able to eliminate the ConvertPK11String call altogether.

Fix that and you have an sr=blizzard

Comment 9

[Ian McGreer]

blizzard: that's what I get for using code from the tree verbatim ;) 
Unfortunately, it's true, nsIEnumerator::next() returns void, or better put,
only notifies the user of a failure via an exception.

The interface is broken, as it notes, but on the other hand I don't see any way
of getting an nsISimpleEnumerator from an nsISupportsArray, without writing my
own function to do it.  I think I'm going to have to leave this as it is, and
wait (along with everyone else) for the proper interface changes.

I'll make the other changes.

-Ian

Comment 10

[Ian McGreer]

preliminary implementation checked in.

I'll mark this as fixed and open more specific bugs as they arise.

Comment 11

[John Unruh]

Verified.