1241646 - remove unused token arguments from nsIX509CertDB (and other interfaces?)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

A number of nsIX509CertDB functions have an argument 'nsISupports aToken' that the underlying implementation completely ignores. We should just remove those arguments for a cleaner interface/implementation. This may be an issue with other interfaces in security/manager/ssl. If so, those should be cleaned up as well.

Comment 1

[simplyblue24]

Attachment: MozReview Request: Bug 1241646 - remove unused token arguments from nsIX509CertDB (and other interfaces?)

Review commit: https://reviewboard.mozilla.org/r/32121/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32121/

Comment 2

[simplyblue24]

Hi, I have removed  argument "nsISupports aToken" from all instances  found in the file "security/manager/ssl/nsIX509CertDB.idl" and their corresponding implementation in the file "security/manager/ssl/nsNSSCertificateDB.cpp". Please do let me know of any other changes need to be done. :)

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8711352 [details]
MozReview Request: Bug 1241646 - remove unused token arguments from nsIX509CertDB (and other interfaces?)

https://reviewboard.mozilla.org/r/32121/#review28995

This is a good start! There are a few items to address.

::: security/manager/ssl/nsIX509CertDB.idl:49
(Diff revision 1)
>  [scriptable, uuid(a36c45fb-f7b5-423e-a0f7-ea1eb4fd60b5)]

This needs to be updated with a new value from 'uuidgen'.

::: security/manager/ssl/nsIX509CertDB.idl:65
(Diff revision 1)
>     *  @param aToken Optionally limits the scope of

Please update the documentation on all of these as well.

::: security/manager/ssl/nsIX509CertDB.idl:255
(Diff revision 1)
> -  void importPKCS12File(in nsISupports aToken,
> +  void importPKCS12File(in nsIFile aFile);

This function actually does use the token, so this argument can't be removed.

::: security/manager/ssl/nsIX509CertDB.idl:268
(Diff revision 1)
> -  void exportPKCS12File(in nsISupports aToken,
> +  void exportPKCS12File(in nsIFile aFile,

Same thing - the token is actually used here.

::: security/manager/ssl/nsNSSCertificateDB.cpp:95
(Diff revision 1)
> -nsNSSCertificateDB::FindCertByNickname(nsISupports *aToken,
> +nsNSSCertificateDB::FindCertByNickname(const nsAString &nickname,

If you're interested, feel free to address some style nits while you're modifying these lines:

& and \* go to the left, as in 'const nsAString& nickname' or 'nsIX509Cert\*\* \_rvCert'.

When wrapping long lines of arguments, the first characters should line up. In this case, the 'n' in 'nsIX509Cert...' should align with the 'c' in 'const nsAString...'

Lines should be less than 80 characters.

There shouldn't be any trailing whitespace (e.g. see line 125).

(See the style guide at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style )

::: security/manager/ssl/nsNSSCertificateDB.cpp:181
(Diff revision 1)
> -nsNSSCertificateDB::FindCertNicknames(nsISupports *aToken, 
> +nsNSSCertificateDB::FindCertNicknames(uint32_t      aType,

Just as a heads-up, I'm simultaneously working on a patch to remove this function.

::: security/manager/ssl/nsNSSCertificateDB.cpp:1066
(Diff revision 1)
> -nsNSSCertificateDB::ImportCertsFromFile(nsISupports *aToken, 
> +nsNSSCertificateDB::ImportCertsFromFile(nsIFile *aFile,

Style: this line will probably be <80 characters if the aType argument is put on the same line.

::: security/manager/ssl/nsNSSCertificateDB.cpp:1322
(Diff revision 1)
> -nsNSSCertificateDB::FindCertByEmailAddress(nsISupports *aToken, const char *aEmailAddress, nsIX509Cert **_retval)
> +nsNSSCertificateDB::FindCertByEmailAddress(const char *aEmailAddress, nsIX509Cert **_retval)

Style: this line is too long

Comment 4

[simplyblue24]

Attachment: 1241646.patch

Had to post the patch here,instead of MozReview, I updated the extension, and then some how it keeps crashing :(

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8712040 [details] [diff] [review]
1241646.patch

Review of attachment 8712040 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good. Just a couple more things to address.

::: security/manager/ssl/nsIX509CertDB.idl
@@ -57,4 @@
>    const unsigned long TRUSTED_SSL     = 1 << 0;
>    const unsigned long TRUSTED_EMAIL   = 1 << 1;
>    const unsigned long TRUSTED_OBJSIGN = 1 << 2;
>  

Up where the nsIX509CertDB interface is defined, you can replace the hex string with this: 5c16cd9b-5a73-47f1-ab0f-11ede7495cce

@@ +58,5 @@
>    const unsigned long TRUSTED_EMAIL   = 1 << 1;
>    const unsigned long TRUSTED_OBJSIGN = 1 << 2;
>  
>    /**
>     *  Given a nickname and optionally a token,

nit: also take out "and optionally a token"

::: security/manager/ssl/nsNSSCertificateDB.cpp
@@ +127,5 @@
>  }
>  
> +NS_IMETHODIMP
> +nsNSSCertificateDB::FindCertByDBKey(const char* aDBkey,
> +                                    nsIX509Cert** _cert)

nit: I think these two can be one line

@@ +199,5 @@
>  
>  NS_IMETHODIMP 
> +nsNSSCertificateDB::FindCertNicknames(uint32_t      aType,
> +                                     uint32_t*     _count,
> +                                     char16_t***  _certNames)

nit: these two lines should move right one more space. Also, there's no need for more than one space between the types and their names.

@@ +1173,2 @@
>                                       uint32_t          count,
> +                                     nsIX509Cert**     certs)

Again, no need for multiple spaces here. And anyway, since this function isn't changing, we probably don't need to address the preexisting style issues here.

@@ +1338,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsNSSCertificateDB::FindCertByEmailAddress(const char *aEmailAddress,
> +                                           nsIX509Cert **_retval)

nit: * to the left here as well

Comment 6

[simplyblue24]

Attachment: 1241646.patch

Hope this looking good now :)

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8712526 [details] [diff] [review]
1241646.patch

Review of attachment 8712526 [details] [diff] [review]:
-----------------------------------------------------------------

Ok - this looks great!
So that's part 1 of this bug. The second part is to update the code that calls any of the functions you modified. You can use dxr to find what you need to change - for example: https://dxr.mozilla.org/mozilla-central/search?q=findCertByNickname&redirect=false&case=false (ignore everything in obj-x86_64-unknown-linux-gnu/ or security/nss/ - most of what you'll need to update is in security/manager/).

Comment 8

[simplyblue24]

Attachment: 1241646.patch

Hi,please let me know, if the changes are any good.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8713057 [details] [diff] [review]
1241646.patch

Review of attachment 8713057 [details] [diff] [review]:
-----------------------------------------------------------------

This is almost perfect, except for two places in C++ code that also need to be updated:

https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/security/manager/ssl/nsNSSIOLayer.cpp#2240 (the call to FindCertByDBKey)
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/security/LocalCertService.cpp#261 (no permalink for this one - looks like dxr is having issues. In any case, it's a call to FindCertByNickname)

::: security/manager/pki/resources/content/viewCertDetails.js
@@ +74,5 @@
>      //var tokenName = "";
>      //var pk11db = Components.classes[nsPK11TokenDB].getService(nsIPK11TokenDB);
>      //var token = pk11db.findTokenByName(tokenName);
>  
>      //var cert = certdb.findCertByNickname(token, myName);

Might as well remove these commented-out lines too.

Comment 10

[simplyblue24]

Attachment: 1241646.patch

Hi, I have made changes.Please review changes.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8713949 [details] [diff] [review]
1241646.patch

Review of attachment 8713949 [details] [diff] [review]:
-----------------------------------------------------------------

Great!
I started a try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3abb246d89c8
(One comment is that since this bug really only addresses nsIX509CertDB, we can remove the '(and other interfaces?)' bit from the patch commit message.)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62d300c9c733

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Try looked good, so I went ahead and checked the patch in. Thanks for working on this!

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62d300c9c733