Closed Bug 1050518 Opened 5 years ago Closed 5 years ago

remove nsICertificatePrincipal

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: keeler, Assigned: Agi)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 1 obsolete file)

nsICertificatePrincipal is a confusingly-named unnecessary wrapper around nsIX509Cert. We do not need it.
Can you flesh out this bug a bit with a DXR link and line or two about what a successful patch looks like?
Flags: needinfo?(dkeeler)
Maybe this is a good next bug, instead. In any case, here's the uses of nsICertificatePrincipal:

http://dxr.mozilla.org/mozilla-central/search?q=nsICertificatePrincipal&case=true&redirect=true

In the case of the interface file and implementation (nsICertificatePrincipal.idl, nsCertificatePrincipal.h, nsCertificatePrincipal.cpp), they can be removed altogether. The other uses will basically be replaced by nsIX509Cert. Method/field names may have to be updated to reflect this (for example, nsIZipReader.getCertificatePrincipal should probably be nsIZipReader.getSigningCert or similar).
Uses of nsIZipReader.getCertificatePrincipal will have to be changed as well to reflect that an nsIX509Cert is returned instead: http://dxr.mozilla.org/mozilla-central/search?q=getCertificatePrincipal&case=true

Things to remember:
* When removing a file, remove its entry from the appropriate moz.build file
* When changing an interface, update its uuid with uuidgen or similar
Flags: needinfo?(dkeeler)
Attached patch Remove nsICertificatePrincipal (obsolete) — Splinter Review
I took a stab at this, it is my first bug in this module :-). David, can you take a look? Thanks!

Try: https://tbpl.mozilla.org/?tree=Try&rev=18f371cf78be
Attachment #8486966 - Flags: review?(dkeeler)
Comment on attachment 8486966 [details] [diff] [review]
Remove nsICertificatePrincipal

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

This looks great! Just a couple of comments.
One general comment is that it might be nice to add braces around any conditional statement that gets touched. Then again, some of those lines are part of multi-line if/else if blocks, so this patch would end up changing a lot more than is strictly necessary. Your call. (If you do decide to do that, I should probably have one last look before this gets checked in.)

::: modules/libjar/nsJAR.cpp
@@ +322,5 @@
>    return rv;
>  }
>  
>  NS_IMETHODIMP
> +nsJAR::GetSigningCert(const nsACString &aFilename, nsIX509Cert** aSigningCert)

nit: while we're here, let's move the '&' to the left: 'const nsACString& aFilename' (this is what the style should be, but old code often doesn't follow the style guidelines)

::: modules/libjar/test/chrome/test_bug386153.html
@@ +46,5 @@
>  // Gets the pretty name from the signing cert or null if the zip is unsigned.
>  function getSigner(zip) {
> +  var signingCert = zip.getSigningCert(null);
> +  if (signingCert)
> +    return signingCert.organization

nit: this line should have a trailing ';'

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +5135,5 @@
>            this.certificate = x509;
>          if (this.certificate && this.certificate.commonName.length > 0)
>            this.certName = this.certificate.commonName;
>          else
> +          this.certName = this.certificate.organization;

I think we can clean this section up a little. First, x509 will always be a Ci.nsIX509, so we can get rid of the conditional there. Then, this.certificate will be set, so no need to test for that. In the end, it will look something like this:

this.certificate = x509;
if (this.certificate.commonName.length > 0) {
  this.certName = this.certificate.commonName;
} else {
  this.certName = this.certificate.organization;
}
Attachment #8486966 - Flags: review?(dkeeler) → review+
Thanks for the quick review David! I addressed your comments and added a few braces in the if-else blocks my patch was directly touching. Thanks!
Assignee: nobody → agi.novanta
Attachment #8486966 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8487668 - Flags: review?(dkeeler)
Comment on attachment 8487668 [details] [diff] [review]
Remove nsICertificatePrincipal v2

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

Great!
Attachment #8487668 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/14febaa4c101
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
See Also: 835179
Duplicate of this bug: 835179
You need to log in before you can comment on or make changes to this bug.