845375 - User Identification Request dialog is unresponsive

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[zebardy]

Attachment: Screenshot_2013-02-26-13-28-21.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20130201065344

Steps to reproduce:

Following a successful import of a client certificate using nsIX509CertDB.importPKCS12File. Browsing to a site which requires the use of the installed client certificate.


Actual results:

This triggers the browser to display a "This site has requested that you identify yourself with a certificate" component (security/manager/pki/resources/content/clientauthask.xul). This cannot be scrolled to reach the accept button, and the certificate chooser drop down is unresponsive.


Expected results:

A native version of this UI component should be displayed, allowing the user to select and confirm the appropriate certificate to use for this site.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

To fix this we need have a native-UI implementation of the nsIClientAuthDialogs interface [1]. The current implementation at [2] tries to open a XUL dialog which doesn't work well on native. The new implementation can join the other PKI-related implementation methods in [3].

[1] http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/public/nsIClientAuthDialogs.idl#20
[2] http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/src/nsNSSDialogs.cpp#273
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/NSSDialogService.js

Comment 2

[zebardy]

For reference ... the implementation of the other native-UI dialogs was carried out as change:- http://hg.mozilla.org/mozilla-central/rev/a8a7bc20e2aa this was in response to bug:- https://bugzilla.mozilla.org/show_bug.cgi?id=807606

Comment 3

[zebardy]

Attachment: First attempt at a patch

Seeing as firefox is open source and I'm taking an interest in this feature, I figured why not have a go myself. This patch works well for me. The only part I'm having real trouble with is the remember checkbox. I can't figure out for the life of me how to replicate in java:-

nsCOMPtr<nsIClientAuthUserDecision> extraResult = do_QueryInterface(ctx);

(from line 281 of http://hg.mozilla.org/mozilla-central/file/a4f834dd884f/security/manager/pki/src/nsNSSDialogs.cpp) 

I'd be interested on any feedback, and assistance with finishing the patch. 

Many Thanks

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 720642 [details] [diff] [review]
First attempt at a patch

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

This is really good! It looks like you figured out almost everything you need. That's pretty awesome for a first patch, specially considering this area of the code isn't the easiest to follow and understand! I have some comments below.

::: mobile/android/components/MobileComponents.manifest
@@ +92,5 @@
>  
>  # NSSDialogService.js
>  component {cbc08081-49b6-4561-9c18-a7707a50bda1} NSSDialogService.js
>  contract @mozilla.org/nsCertificateDialogs;1 {cbc08081-49b6-4561-9c18-a7707a50bda1}
> +component {b4aef975-385b-47c7-b6c0-b90af9d9dfe3} NSSDialogService.js

You can get rid of this line; see further comments below about merging the two interfaces.

::: mobile/android/components/NSSDialogService.js
@@ +137,5 @@
> +
> +NSSClientAuthDialogs.prototype = {
> +  classID: Components.ID("{b4aef975-385b-47c7-b6c0-b90af9d9dfe3}"),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIClientAuthDialogs]),
> +

You can merge this implementation into the NSSDialogs object. You just need to modify the QueryInterface of NSSDialogs to look like this:

  QueryInterface: XPCOMUtils.generateQI([Ci.nsICertificateDialogs, Ci.nsIClientAuthDialogs])

See the PromptService.js file in this folder for an example. Although doing this is a little less modular I prefer it in this case because it will reuse the helper functions (getString, showPrompt), and the code is actually pretty related.

@@ +147,5 @@
> +  },
> +
> +  formatString: function(aName, argList) {
> +    if (!this.bundle) {
> +        this.bundle = Services.strings.createBundle("chrome://browser/locale/pippki.properties");

Try to stick to two-space indentation throughout this file to be consistent.

@@ +173,5 @@
> +                      }
> +                    ]);
> +  },
> +
> +  ChooseCertificate: function(aCtx, cn, organization, issuer, certNickList, certDetailsList, count, selectedIndex, canceled) {

This should be "chooseCertificate" (with a lowercase c at the front)

@@ +181,5 @@
> +    if (pref) {
> +      pref = pref.getBranch(null);
> +      try {
> +    rememberSetting = 
> +      pref.getBoolPref("security.remember_cert_checkbox_default_setting");

Define "let rememberSetting = true;" above the "var pref = ..." line, so that the variable is still defined to something reasonable if this block fails or is never entered. Also the indentation here seems wrong.

@@ +188,5 @@
> +    // pref is missing
> +      }
> +    }
> +   
> +    let organizationString = this.formatString("clientAuthAsk.organization", 

There are some trailing spaces on the above two lines (they show up in red in splinter view) - please remove them.

@@ +192,5 @@
> +    let organizationString = this.formatString("clientAuthAsk.organization", 
> +                                               [organization]);
> +    let issuerString = this.formatString("clientAuthAsk.issuer",
> +                                         [issuer]);
> +    let serverRequestedDetails = cn+'<br/>'+organizationString+'<br/>'+issuerString;

Add spaces around each '+' character here.

@@ +197,5 @@
> +
> +    selectedIndex = 0;
> +    while (true) {
> +      let response = this.showPrompt(this.getString("clientAuthAsk.title"),
> +                                    this.getString("clientAuthAsk.message1"),

Indent one more space here

@@ +203,5 @@
> +                                      this.getString("clientAuthAsk.viewCert.label"),
> +                                      this.getString("nssdialogs.cancel.label")
> +                                    ],
> +                                    [ { type: "label", id: "requestedDetails", label: serverRequestedDetails},
> +                                      { type: "menulist", id: "nicknames", label: this.getString("clientAuthAsk.message2"), values: certNickList, selected: selectedIndex},

The above two lines should have a space before '}'

@@ +215,5 @@
> +        canceled.value = false;
> +        if (response.rememberBox == "true") {
> +//          need to convert nsCOMPtr<nsIClientAuthUserDecision> extraResult = do_QueryInterface(ctx);
> +//          into javasript
> +//          CcId['95c4373e-bdd4-4a63-b431-f5b000367721'].getService(Ci.nsIClientAuthUserDecision).SetRememberClientAuthCertificate(true);

I believe the way to do this is like so:

aCtx.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIClientAuthUserDecision).rememberClientAuthCertificate = true;

The first bit ("QueryInterface" and "getInterface") is like casting the aCtx into a nsIClientAuthUserDecision, and then you can set the property on it. I'm no expert on this though, so you'll have to test it to make sure.

@@ +229,5 @@
> +const components = [
> +                     NSSDialogs,
> +                     NSSClientAuthDialogs
> +                   ];
> +this.NSGetFactory = XPCOMUtils.generateNSGetFactory(components);

You should be able to revert these lines back to the original

Comment 5

[zebardy]

Attachment: updated patch

Comment 6

[zebardy]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 720642 [details] [diff] [review]
> First attempt at a patch
> 
> Review of attachment 720642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is really good! It looks like you figured out almost everything you
> need. That's pretty awesome for a first patch, specially considering this
> area of the code isn't the easiest to follow and understand! I have some
> comments below.

Thanks, it was an interesting learning experience. I'm not a javascript coder, but it's good to try things in new languages every so often. I've read through your review made a few changes and figured out how to get the checkbox working thanks to your hint. Still wondering if there are any test scripts for all of this. I've been resorting to rebuilding, manually testing and watching the logcat.

> 
> ::: mobile/android/components/MobileComponents.manifest
> @@ +92,5 @@
> >  
> >  # NSSDialogService.js
> >  component {cbc08081-49b6-4561-9c18-a7707a50bda1} NSSDialogService.js
> >  contract @mozilla.org/nsCertificateDialogs;1 {cbc08081-49b6-4561-9c18-a7707a50bda1}
> > +component {b4aef975-385b-47c7-b6c0-b90af9d9dfe3} NSSDialogService.js
> 
> You can get rid of this line; see further comments below about merging the
> two interfaces.
> 
> ::: mobile/android/components/NSSDialogService.js
> @@ +137,5 @@
> > +
> > +NSSClientAuthDialogs.prototype = {
> > +  classID: Components.ID("{b4aef975-385b-47c7-b6c0-b90af9d9dfe3}"),
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIClientAuthDialogs]),
> > +
> 
> You can merge this implementation into the NSSDialogs object. You just need
> to modify the QueryInterface of NSSDialogs to look like this:
> 
>   QueryInterface: XPCOMUtils.generateQI([Ci.nsICertificateDialogs,
> Ci.nsIClientAuthDialogs])
> 
> See the PromptService.js file in this folder for an example. Although doing
> this is a little less modular I prefer it in this case because it will reuse
> the helper functions (getString, showPrompt), and the code is actually
> pretty related.
> 
> @@ +147,5 @@
> > +  },
> > +
> > +  formatString: function(aName, argList) {
> > +    if (!this.bundle) {
> > +        this.bundle = Services.strings.createBundle("chrome://browser/locale/pippki.properties");
> 
> Try to stick to two-space indentation throughout this file to be consistent.

Apologies I'm mostly working in perl at the moment and have my tabspace set to 4

> 
> @@ +173,5 @@
> > +                      }
> > +                    ]);
> > +  },
> > +
> > +  ChooseCertificate: function(aCtx, cn, organization, issuer, certNickList, certDetailsList, count, selectedIndex, canceled) {
> 
> This should be "chooseCertificate" (with a lowercase c at the front)
> 
> @@ +181,5 @@
> > +    if (pref) {
> > +      pref = pref.getBranch(null);
> > +      try {
> > +    rememberSetting = 
> > +      pref.getBoolPref("security.remember_cert_checkbox_default_setting");
> 
> Define "let rememberSetting = true;" above the "var pref = ..." line, so
> that the variable is still defined to something reasonable if this block
> fails or is never entered. Also the indentation here seems wrong.
> 
> @@ +188,5 @@
> > +    // pref is missing
> > +      }
> > +    }
> > +   
> > +    let organizationString = this.formatString("clientAuthAsk.organization", 
> 
> There are some trailing spaces on the above two lines (they show up in red
> in splinter view) - please remove them.
> 
> @@ +192,5 @@
> > +    let organizationString = this.formatString("clientAuthAsk.organization", 
> > +                                               [organization]);
> > +    let issuerString = this.formatString("clientAuthAsk.issuer",
> > +                                         [issuer]);
> > +    let serverRequestedDetails = cn+'<br/>'+organizationString+'<br/>'+issuerString;
> 
> Add spaces around each '+' character here.
> 
> @@ +197,5 @@
> > +
> > +    selectedIndex = 0;
> > +    while (true) {
> > +      let response = this.showPrompt(this.getString("clientAuthAsk.title"),
> > +                                    this.getString("clientAuthAsk.message1"),
> 
> Indent one more space here
> 
> @@ +203,5 @@
> > +                                      this.getString("clientAuthAsk.viewCert.label"),
> > +                                      this.getString("nssdialogs.cancel.label")
> > +                                    ],
> > +                                    [ { type: "label", id: "requestedDetails", label: serverRequestedDetails},
> > +                                      { type: "menulist", id: "nicknames", label: this.getString("clientAuthAsk.message2"), values: certNickList, selected: selectedIndex},
> 
> The above two lines should have a space before '}'
> 
> @@ +215,5 @@
> > +        canceled.value = false;
> > +        if (response.rememberBox == "true") {
> > +//          need to convert nsCOMPtr<nsIClientAuthUserDecision> extraResult = do_QueryInterface(ctx);
> > +//          into javasript
> > +//          CcId['95c4373e-bdd4-4a63-b431-f5b000367721'].getService(Ci.nsIClientAuthUserDecision).SetRememberClientAuthCertificate(true);
> 
> I believe the way to do this is like so:
> 
> aCtx.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.
> nsIClientAuthUserDecision).rememberClientAuthCertificate = true;
> 
> The first bit ("QueryInterface" and "getInterface") is like casting the aCtx
> into a nsIClientAuthUserDecision, and then you can set the property on it.
> I'm no expert on this though, so you'll have to test it to make sure.
> 
> @@ +229,5 @@
> > +const components = [
> > +                     NSSDialogs,
> > +                     NSSClientAuthDialogs
> > +                   ];
> > +this.NSGetFactory = XPCOMUtils.generateNSGetFactory(components);
> 
> You should be able to revert these lines back to the original

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 721269 [details] [diff] [review]
updated patch

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

This looks good to me, just fix up the formatting pointed out below and I can land it. As for testing, unfortunately we don't have a lot of testing coverage for this sort of code in the new "native fennec". Exercising the code manually and making sure it has the desired behaviour is currently the only way to really test it. Thanks for fixing this!

::: mobile/android/components/NSSDialogService.js
@@ +158,5 @@
> +      try {
> +        rememberSetting = pref.getBoolPref("security.remember_cert_checkbox_default_setting");
> +      }
> +      catch(e) {
> +    // pref is missing

Put the catch on the same line as the previous '}' and indent the comment a bit more so it lines up with the body of the try block.

@@ +161,5 @@
> +      catch(e) {
> +    // pref is missing
> +      }
> +    }
> + 

Remove trailing space on this line

Comment 8

[zebardy]

Attachment: updated following further review

Updated patch ... hopefully this is it :)

Comment 9

[zebardy]

Attachment: updated as I forgot about the trailing white space

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 721822 [details] [diff] [review]
updated as I forgot about the trailing white space

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

Whoops, I lost my review comment when trying to submit around the bugzilla downtime. I also just noticed that the patch doesn't have your author information or a commit message. You can either re-generate the patch with that as per https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in or just let me know what name and/or email you'd like to be used as the author, and I can land the patch with that and the fix below.

::: mobile/android/components/NSSDialogService.js
@@ +156,5 @@
> +      pref = pref.getBranch(null);
> +      try {
> +        rememberSetting = pref.getBoolPref("security.remember_cert_checkbox_default_setting");
> +      }
> +        catch(e) {

Sorry for the confusion, I meant it should be like:

  rememberSetting = ...
} catch (e) {
  // ...

Comment 11

[zebardy]

Ok, I think I need some more mercurial queues fu. I think the simplest solution is to give you the author information:-

Name: Aaron Moses
Email: [e-mail]

Thanks

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Awesome! I updated the patch and landed it:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffe335c066f6

Thanks again for sticking with this bug and implementing it :) This bug will be marked as fixed once the patch gets merged to mozilla-central.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ffe335c066f6