168450 - Cleanup some PSM code and add JavaDoc documentation to all freeze candidates

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Kai Engert (:KaiE:)]

The following interfaces require complete JavaDoc style documentation:

nsICRLInfo
nsIX509Cert
nsIX509CertDB
nsIX509CertValidity
nsIASN1Sequence
nsIASN1Object
nsICertificateDialogs
nsIBadCertListener
nsISecurityWarningDialogs

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

This patch:

- adds JavaDoc style comments for all elements in all interfaces we are going
to freeze

- it also fixes bug 169148 (change return type of caCertExists)

- I reordered the elements in nsIX509Cert to a more logical grouping, but I did
not change anything within that interface


Javi, can you please review?

Comment 2

[Kai Engert (:KaiE:)]

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

Comment 3

[Javier Delgadillo]

Comment on attachment 99684 [details] [diff] [review]
Patch v1

 interface nsICRLInfo : nsISupports {
+
+  /**
+   *  The issueing CA's organization.
+   */
   readonly attribute wstring org;
+
+  /**
+   *  The issueing CA's organizational unit.
+   */
   readonly attribute wstring orgUnit;

correct mis-spelling issueing->issuing in both comments

I assume the following line will disappear from the nsIX509Cert interface:

readonly attribute wstring rsaPubModulus;

+  /**
+   *  Obtain an single comma 

Should say "Obtain a single comma..."

Comment 4

[Javier Delgadillo]

Comment on attachment 99684 [details] [diff] [review]
Patch v1

 interface nsICRLInfo : nsISupports {
+
+  /**
+   *  The issueing CA's organization.
+   */
   readonly attribute wstring org;
+
+  /**
+   *  The issueing CA's organizational unit.
+   */
   readonly attribute wstring orgUnit;

correct mis-spelling issueing->issuing in both comments

I assume the following line will disappear from the nsIX509Cert interface:

readonly attribute wstring rsaPubModulus;

+  /**
+   *  Obtain an single comma 

Should say "Obtain a single comma..."

Should we just get rid of this line following exportPKCS12File?

//[array, size_is(count)] in wstring aCertNames);

perhaps the comments for getOCSPResponders should say what to QI the elements
in the returned nsISupportsArray to.  I'm assuming nsIX509Cert, but I could be
wrong.	

Other than that, r=javi

Comment 5

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 6

[Kai Engert (:KaiE:)]

> correct mis-spelling issueing->issuing in both comments

done


> I assume the following line will disappear from the nsIX509Cert interface:
> readonly attribute wstring rsaPubModulus;

yeah, see bug 168976


> Should say "Obtain a single comma..."

fixed


> Should we just get rid of this line following exportPKCS12File?
> //[array, size_is(count)] in wstring aCertNames);

indeed, removed


> perhaps the comments for getOCSPResponders should say what to QI the elements
> in the returned nsISupportsArray to.  I'm assuming nsIX509Cert, but I could be
> wrong.	

agreed, added the comment
 @return Array of OCSP responders, entries are QIable to nsIOCSPResponder

Comment 7

[Kai Engert (:KaiE:)]

Attachment: Patch v2

This is the real patch v2 file.

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 99701 [details] [diff] [review]
Patch v2

Marking r=javi since I addresses his comments.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 99701 [details] [diff] [review]
Patch v2

Marking r=javi since I addressed his comments.

Comment 10

[Kai Engert (:KaiE:)]

Alec, can you please review?

Comment 11

[timeless]

Comment on attachment 99701 [details] [diff] [review]
Patch v2

i'm going to send kaie a list.

Comment 12

[Kai Engert (:KaiE:)]

Sigh, is my patch really that bad? Timeless sent me a message where he requests
to change nearly every comment, and even some semantics and interface names. I
feel a bit reluctant to spend another day on that cleanup.

Comment 13

[Alec Flett]

Comment on attachment 99701 [details] [diff] [review]
Patch v2

kaie: timeless is extremely pedantic, but we do appreciate his thoroughness. 

Timeless, can you tone it down a little and post the toned-down comments to the
bug? By toned down, I mean try to use some judgement about what issues are
really important right now, and what issues are just nitpicks - it is more
important to fix the larger issues than the nitpicks.

If timeless's issues with the patch are really that long, then there is either
a more global problem (i.e. timeless, you don't have to list every instance of
a misspelled or misused word) or there is something wrong with the whole patch.

I'm willing to be the mediator on this, but lets try to figure out what issues
are most important.

Comment 14

[timeless]

Comment on attachment 99701 [details] [diff] [review]
Patch v2

This is really a review of the API using the documentation as a guide.

A. there are a bunch of apis where you use true to mean cancel
B. and then a bunch of apis where false means cancel (and true means continue).
 [I'd prefer true to mean continue.]
  * In fact perhaps "don't continue" should be handled as an exception instead
of a return. consider:
   boolean getPKCS12FilePassword(in nsIInterfaceRequestor ctx, 
                                 out wstring password);
   boolean setPKCS12FilePassword(in nsIInterfaceRequestor ctx,
                                 out wstring password); 
it's really wstring promptForPassword/choosePassword(in nsIInterfaceRequestor
ctx, long type[PKCS12_File=1]) @throws canceled

C. functions named |alertWhatever| which are designed to confirm things should
be |confirmWhatever|. Similarly functions which find things shouldn't be
|getThing| they should be |findThing|.
  * boolean unknownIssuer/mismatchDomain. without the idl comment you'd have no
idea that this is a confirm function, it needs a better name. I think it's an
allow/use/accept.
  * cACertExists should probably be a confirm but at least it shouldn't begin cA

D. there are a bunch of human readable fields on the interfaces, do they map to
distinct parts of the certificates? If they don't (i.e. they map to the same
datcertificate field as the non human readable interface item), then do they
need to live on the interface? especially the time fields, for which the
consumer can use a normal time to human readable api.

E. there are a bunch of rather similar search functions, i'm wondering if it
might make sense to have a single function with all of the search fields instead
of lots of similar functions, or perhaps only one search field since they're all
strings and a search type field.

F. some functions have optional parameters first, (G) some have them last. [i'd
prefer for them to be last (right before out/retvals)]

H. some of the boolean attributes (processObjects, showObjects) are asking for a
prefix like canProcessObjects. - But I don't understand how showObjects would 
be used so I have no idea what sort of thing it is...

I. ADD_<whatever>, some of these things aren't really adds...

J. org/orgUnit v. organization/organizationalUnit - unless you think someone
will be implementing both of these in a single object i think you should be
consistent and use the same attribute names.

K. don't use 'usages'.

L. functions which could be replaced by a simple attribute:
    readonly attribute boolean ocspOn;
    void enableOCSP();
=> attribute boolean ocspEnabled;

    void getRawDER(out unsigned long length, [retval, array, size_is(length)]
out octet data); -- couldn't this be a readonly attribute?

* misc
  * boolean isSameCert(in nsIX509Cert other) - Why can't this be called equals?
  * void deleteCertificate(in nsIX509Cert aCert) - this is removeCert[ificate]
-- consistency ...
  * boolean getCertTrust(in nsIX509Cert cert,...) - isCertTrusted/Trustworthy

AC  boolean alertEnteringSecure(in nsIInterfaceRequestor ctx);
AC  boolean alertEnteringWeak(in nsIInterfaceRequestor ctx);
AC  boolean alertLeavingSecure(in nsIInterfaceRequestor ctx);
AC  boolean alertMixedMode(in nsIInterfaceRequestor ctx);
A   boolean confirmPostToInsecure(in nsIInterfaceRequestor ctx);
A   boolean confirmPostToInsecureFromSecure(in nsIInterfaceRequestor ctx);
BC  boolean unknownIssuer(in nsIInterfaceRequestor socketInfo, 
                          in nsIX509Cert cert,
                          out short certAddType);
BC  boolean mismatchDomain(in nsIInterfaceRequestor socketInfo,
                           in wstring targetURL,
                           in nsIX509Cert cert);
B   boolean certExpired(...)
B   boolean downloadCACert(...)
 C  cACertExists(...)

D   readonly attribute wstring lastUpdateLocale;
D   readonly attribute wstring nextUpdateLocale;
D   readonly attribute wstring issuedDate;
D   readonly attribute wstring issuedDateSortable;
D   readonly attribute wstring expiresDate;
D   readonly attribute wstring expiresDateSortable;

ECF nsIX509Cert getCertByNickname(in nsISupports aToken,
                                  in wstring aNickname);
ECG nsIX509Cert getCertByDBKey(in string aDBkey, in nsISupports aToken);
 C  void getCertNicknames(in nsISupports aToken, 
                          in unsigned long aType,
                          out unsigned long count,
                          [array, size_is(count)] out wstring certNameList);
EC  nsIX509Cert getEmailEncryptionCert(in wstring aNickname);
EC  nsIX509Cert getEmailSigningCert(in wstring aNickname);
ECF nsIX509Cert getCertByEmailAddress(in nsISupports aToken,
                                      in string aEmailAddress);
H   attribute boolean processObjects;
H   attribute boolean showObjects;
HL  readonly attribute boolean usesOCSP;
I   const short ADD_TRUSTED_FOR_SESSION = 1; (TRUST_FOR_SESSION)
I   const short ADD_TRUSTED_PERMANENTLY = 2; (TRUST_PERMANENTLY)

Comment 15

[Alec Flett]

ok, my take on timeless's comments: They are valid in that the issues he's
brought up with the API will change the way the APIs are documented if the
issues are addressed. I think its better to fix the interface first, then fix
the documentation, rather than fix documentation now, change the APIs, and
redocument later, but I'll leave that up to Kai.

A. and B. - if he's right, yes, we should clean these up to be consistent.
C. I also agree, on all points.
D. we've discussed these previously with Kai, and we're going to leave them as
they are for now.
E. I think the API is fine as it is. Unless Kai can come up with an advantage to
having one function, lets leave it
F. I think kai should make this call. We want consistent, clear APIs but I'm
assuming that the existing ordering is fine. since there is no technical way to
have  "optional" methods, I consider passing in null to be a valid use of the
parameter, with a particular meaning. It doesn't mean that the parameter is
"optional"
H. I'll let kai explain the interface. In general, we like the attribute name to
allow the code to be more human readable. I don't know the context in which to
compare foo.processObjects vs. foo.canProcessObjects. My guess though, is that
timeless is correct that these should have the "can" part.

So overall I think whats happened is that we (Myself, Kai, Juan, etc) have
looked at the individual methods to see if they seem valid, but haven't looked
at the whole set of interfaces as a cohesive unit. Timeless has provided some
valid feedback here that we should address regarding consistency, etc.

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 99701 [details] [diff] [review]
Patch v2

I disagree.

The suggested changes do not decrease dependencies, nor do they increase
functionality.
Especially I fear the risk of confusion and regressions caused by reverting the
meaning of true/false in existing APIs.

But this is not the topic of this bug.

If you think we should change the current API, please file separate coding
style enhancement bugs.

This bug is about adding documentation to the interfaces, which is the task
that has been assigned to me.

I have done that.

If some of my explanations are not good enough, that might be because I don't
know it better, as I didn't write the underlying code and didn't study it fully
yet. The comments are meant to provide help understanding the APIs as they are
now.

Comment 17

[Alec Flett]

somehow, my comments for J, K, and L didn't make it above - I think they are
also valid points. "usages" isn't even a word.

Comment 18

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Stephane allowed me to spend some resources on this. With the new patch I have
addressed your comments that I agree with. I have commented on the parts I
disagree with.

Please review the new patch. What I changed is explained below.


> A. there are a bunch of apis where you use true to mean cancel

Only my comment was wrong for the two confirm... functions.
Changed for the alert... functions.


> B. and then a bunch of apis where false means cancel (and true means
continue).
>  [I'd prefer true to mean continue.]

Reread your statement, what are you saying? You prefer if it were as it is???
;-)
I assume you still want to use true to mean continue...

I changed the get/setpkcs12 funcs.
I changed DownloadCACert.
For the other functions you listed, true already means continue...


> C. functions named |alertWhatever| which are designed to confirm things
should
> be |confirmWhatever|. Similarly functions which find things shouldn't be
> |getThing| they should be |findThing|.

renamed


>   * boolean unknownIssuer/mismatchDomain. without the idl comment you'd have
no
> idea that this is a confirm function, it needs a better name. I think it's an

> allow/use/accept.

renamed


>   * cACertExists should probably be a confirm but at least it shouldn't begin
cA

renamed to say notifyCACertExists



> D. we've discussed these previously with Kai, and we're going to leave them
as
> they are for now.

nothing changed


> E. I think the API is fine as it is. Unless Kai can come up with an advantage
to
> having one function, lets leave it

nothing changed


> F. I think kai should make this call.

nothing changed


> H. some of the boolean attributes (processObjects, showObjects) are asking
for a
> prefix like canProcessObjects. - But I don't understand how showObjects would

> be used so I have no idea what sort of thing it is...

You didn't seem to have read my interface comments, in it I explained what
showObjects means.
This interface enables the object to be used directly as a data structure for a
visible 
represenatation implementation.
Anyway, renamed to something that is hopefully clearer.
processObjects -> isValidContainer
showObjects -> isExpanded


> I. ADD_<whatever>, some of these things aren't really adds...

You are wrong. These constants are used when *trust is added* to the database.
Not changed.


> J. org/orgUnit v. organization/organizationalUnit - unless you think someone
> will be implementing both of these in a single object i think you should be
> consistent and use the same attribute names.

renamed


> K. don't use 'usages'.

I'm not a native speaker so I can not tell whether this word is bad or not.
Based on a suggestion from timeless on IRC, I renamed "usages" to "uses"
and "usage" to "use".


> L. functions which could be replaced by a simple attribute:
>     readonly attribute boolean ocspOn;
>     void enableOCSP();
> => attribute boolean ocspEnabled;

No.
You obviously did not read the comments that I added in the patch.
The functions do not change the attribute.
They are used to override the attribute, as it is explained in the comments...
Not changed.


> * misc
>   * boolean isSameCert(in nsIX509Cert other) - Why can't this be called
equals?

renamed


>   * void deleteCertificate(in nsIX509Cert aCert) - this is
removeCert[ificate]

No, not changed. We delete.


>   * boolean getCertTrust(in nsIX509Cert cert,...) - isCertTrusted/Trustworthy


renamed

Comment 19

[timeless]

> Reread your statement, what are you saying? You prefer if it were as it is???
sorry. I should have said "I prefer B over A".

Whether OCSP is enabled in preferences.
   readonly attribute boolean ocspOn;
Use this to temporarily disable OCSP checking. Needed if OCSP checks slow down
UI rendering too much. A call to this should be followed with a call to
enableOCSP soon afterwards.
   void disableOCSP();
Sets the OCSP options to correspond with the preferences values.
   void enableOCSP();

Eek you're right On and Enable aren't a pair. I have no idea how i missed
disable. So, there are three, and the correct pair is obviously disabled/enable,
could that be: 
attribute boolean skipOscp; //?

Also, 'temporarily' usually is kind of a flag like /* XXX hack fix me before
mozilla0.9.2, (arbitrary outdated milestone to indicate that the problem was
forgotten) */
Does it really belong on the frozen interface? If at some point you fix it so
that it isn't needed, then you will have these useless methods hanging off your
interface.

fwiw, after reading the patch (which i like a lot), i think 'allow' is more
appropriate than 'confirm'.

Kaie: thanks for the work.

Comment 20

[Kai Engert (:KaiE:)]

Attachment: Patch v4

Your comment regarding the skip OCSP mode is good.
I agree this mode looks like a hack.
I saw that PSM only uses this hack in one particular situation, and therefore I
have changed the code to no longer offer to enable a global skip mode, but
rather only offer the single function with the ability to skip OCSP using a
boolean parameter.

Comment 21

[timeless]

Comment on attachment 105455 [details] [diff] [review]
Patch v4

those changes look great :)

Comment 22

[Kai Engert (:KaiE:)]

Javi, can you please review?

Comment 23

[Javier Delgadillo]

Comment on attachment 105455 [details] [diff] [review]
Patch v4

r=javi

Comment 24

[Kai Engert (:KaiE:)]

Alec, can you please review?

Comment 25

[Alec Flett]

Comment on attachment 105455 [details] [diff] [review]
Patch v4

this generally looks ok, the one thing I'm now confused on (and maybe its just
been a while) is the "issuedDateSortable". I remember that most of the
string-based dates were only for display, but if this is a sorting one, why
can't this be a PRTime type? those are totally sortable, and independent of
timezone, etc. (for instance, what timezone is issuedDateSortable - is it GMT
or the current TZ?)

in any case, that's more an issue for another pass at these interfaces, I
suppose. 

Thanks for this cleanup. This API is much clearer already. sr=alecf if you put
a comment next to the 
issuedDateSortable saying:
1) the timezone of the string
2) that it should maybe be PRTime?

Comment 26

[Kai Engert (:KaiE:)]

I would like to add a final statement about the term "usage".

I fully disagree with your request to change it from "usage" to "use".
I was believing you, because I'm not a native english speaker.

However, I meanwhile see that he crypto library uses the term "usage" all over
the place in its sourcecode.
The X.509 RFC documents speak of "key usage" in the same contexts.
I asked our technical writer about this phrase, and it seems OK to him, too.

With the patch in this bug, I'm making a change that I really dislike.
I'm going away from the term usage, that is used everywhere else.

For people new to the code, that might make it harder for them to understand
that what we call "use" in PSM is actually the same thing that is described as
"usage" in NSS and in the RFCs.

I feel tempted to change it back and I wish I had researched this better before
I worked on this new patch.

However, I don't want to spend yet more time on this nit, and to not lose more
time discussing this small nit, I will not change the patch again with regards
to "usage".

I will attach another patch soon that addresses Alec's comments.

Comment 27

[Kai Engert (:KaiE:)]

> why can't this be a PRTime type

We talked about this before, but not yet for the certificate, only for the CRL
interface.

The problem is that we need to work with the string based representation of this
attribute from within JS. When I looked at it, I did not find a simple way to do
the string formatting from PRTime to a string from within JS.

For the CRL interface, I solved the problem by offering both, the time as PRTime
and as a formatted string.

I just looked what can be done. The sortable representation is only needed from
C++. I can do the following:
- remove the string_sortable attributes from the interface
- add PRTime attributes, and make the consumer C++ code to do the string
formatting (the general cert UI column code still needs the date formatted as a
string)
- document the other attribute is a locale formatted string in local time

Comment 28

[Kai Engert (:KaiE:)]

Attachment: Patch v5

Patch grew by 10%.

I removed the time attributes from nsIX509Cert completely, since it contains a
validity attribute already, which is the place where all time information
should be queried from. I added the ability to obtain a day-only string from
the validity and changed the consumers accordingly and as described in the
previous comment.

Comment 29

[Alec Flett]

Comment on attachment 106025 [details] [diff] [review]
Patch v5

thanks for cleaning up the sortable date issue.. 

as for usage, obviously that aspect of the API is up to you, but at least
realize what you're saying: that you prefer that the API use the term "usage"
but you refuse to change it. I don't know what to make of it since the use of
"Usage" here is not gramatically correct, but if Usage has become a well
understood term in the world of security, it seems like it would be worth the
effort to get the API right. Otherwise, you'll have to spend just as much work
fixing the documentation in the IDL to indicate that Uses == Usages.

Up to you. sr=alecf on this patch, but you can also change Uses to Usages and
the sr= will still apply.

Comment 30

[Kai Engert (:KaiE:)]

Attachment: Patch v5b

Thanks for the reviews.

This is the same patch as v5, but again using the term usage.

Comment 31

[Kai Engert (:KaiE:)]

Checked in to trunk, marking fixed.

Comment 32

[John Unruh]

Verified per kaie's comment.