1376587 - Update GeckoView.ProgressListener to better expose information

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Dylan Roeh (:droeh) (he/him)]

Rather than just exposing an opaque integer, we probably want to expose something a bit more usable in ProgressListener.onSecurityChange().

Comment 1

[Dylan Roeh (:droeh) (he/him)]

Attachment: Expose the bundle generated by IdentityHandler in ProgressListener

First shot at a patch, still a few odds and ends to wrap up. Basically takes what we do to update a SiteIdentity in Fennec and modifies it to be usable by GeckoView, then passes the resulting GeckoBundle to ProgressListener.onSecurityChange()

Comment 2

[Dylan Roeh (:droeh) (he/him)]

Attachment: Expose the bundle generated by IdentityHandler in ProgressListener

Slight update to remove some unnecessary logging, otherwise same as above.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8881548 [details] [diff] [review]
Expose the bundle generated by IdentityHandler in ProgressListener

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +1085,5 @@
>  
>          /**
>          * The security status has been updated.
>          * @param view The GeckoView that initiated the callback.
>          * @param status The new security status.

Please document exactly what the `identity` bundle contains.

::: mobile/android/modules/geckoview/GeckoViewProgress.jsm
@@ +76,5 @@
> +  /**
> +   * Helper to parse out the important parts of _lastStatus (of the SSL cert in
> +   * particular) for use in constructing identity UI strings
> +  */
> +  getIdentityData : function() {

Maybe pass in `lastStatus` as an argument, and get rid of `this._lastStatus`

@@ +117,5 @@
> +    if (aState & Ci.nsIWebProgressListener.STATE_IS_SECURE) {
> +      return this.IDENTITY_MODE_IDENTIFIED;
> +    }
> +
> +    // We also allow "about:" by allowing the selector to be empty (i.e. '(|.....|...|...)'

Don't need this block for GeckoView.

@@ +140,5 @@
> +  },
> +
> +  getMixedActiveMode: function getActiveDisplayMode(aState) {
> +    // Only show an indicator for loaded mixed content if the pref to block it is enabled
> +    // TODO: This?

What are we supposed to do here?

@@ +155,5 @@
> +  },
> +
> +  getTrackingMode: function getTrackingMode(aState, aBrowser) {
> +    if (aState & Ci.nsIWebProgressListener.STATE_BLOCKED_TRACKING_CONTENT) {
> +      this.shieldHistogramAdd(aBrowser, 2);

Don't need these calls

@@ +204,5 @@
> +    }
> +    this._lastLocation = locationObj;
> +
> +    let uri = aBrowser.currentURI;
> +    //TODO: this

What are we supposed to do here?

@@ +240,5 @@
> +
> +    result.host = this.getEffectiveHost();
> +
> +    let iData = this.getIdentityData();
> +    //TODO: This

What are we supposed to do here?

@@ +253,5 @@
> +      if (iData.city) {
> +        supplemental += iData.city + "\n";
> +      }
> +      if (iData.state && iData.country) {
> +        //TODO: This

What are we supposed to do here?

@@ +293,5 @@
> +
> +  /**
> +   * Attempt to provide proper IDN treatment for host names
> +   */
> +  getEffectiveHost: function getEffectiveHost() {

Pass `lastLocation` as an argument, and get rid of `this._lastLocation`

@@ +298,5 @@
> +    if (!this._IDNService)
> +      this._IDNService = Cc["@mozilla.org/network/idn-service;1"]
> +                         .getService(Ci.nsIIDNService);
> +    try {
> +      return this._IDNService.convertToDisplayIDN(this._uri.host, {});

`this._uri` is not set

Comment 4

[Dylan Roeh (:droeh) (he/him)]

Attachment: Expose the bundle generated by IdentityHandler in ProgressListener v1.1

Sorry about that, wrote those TODOs for myself and then forgot to search for them before putting up the patch. I fixed some and left others in place with actual messages instead of placeholders, the main issue being Strings usage. I think I addressed everything else as well.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8882449 [details] [diff] [review]
Expose the bundle generated by IdentityHandler in ProgressListener v1.1

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +1122,5 @@
> +        *          "supplemental": A string containing supplemental information (city, state)
> +        *          "country": A string containing the country associated with the certificate
> +        *          "securityException": A boolean indicating if the site is a security exception
> +        *          "mode": A GeckoBundle containing information about the security mode, with keys:
> +        *            "identity": A string indicating the security level of the site, should always be present

List possible values in the doc for all these mode keys.

::: mobile/android/modules/geckoview/GeckoViewProgress.jsm
@@ +32,5 @@
> +  // Extended-Validation SSL CA-signed identity information (EV). A more rigorous validation process.
> +  IDENTITY_MODE_VERIFIED: "verified",
> +
> +  // Part of the product's UI (built in about: pages)
> +  IDENTITY_MODE_CHROMEUI: "chromeUI",

Not using this

@@ +53,5 @@
> +  // No tracking content information. No tracking content icon is shown.
> +  TRACKING_MODE_UNKNOWN: "unknown",
> +
> +  // Blocked active tracking content. Shield icon is shown, with a popup option to load content.
> +  TRACKING_MODE_CONTENT_BLOCKED: "tracking_content_blocked",

Use "blocked"

@@ +56,5 @@
> +  // Blocked active tracking content. Shield icon is shown, with a popup option to load content.
> +  TRACKING_MODE_CONTENT_BLOCKED: "tracking_content_blocked",
> +
> +  // Loaded active tracking content. Yellow triangle icon is shown.
> +  TRACKING_MODE_CONTENT_LOADED: "tracking_content_loaded",

Use "loaded"

@@ +75,5 @@
> +   * particular) for use in constructing identity UI strings
> +  */
> +  getIdentityData : function(lastStatus) {
> +    let result = {};
> +    let status = lastStatus.QueryInterface(Components.interfaces.nsISSLStatus);

Ci.nsISSLStatus

@@ +82,5 @@
> +    // Human readable name of Subject
> +    result.subjectOrg = cert.organization;
> +
> +    // SubjectName fields, broken up for individual access
> +    if (cert.subjectName) {

Don't bother parsing subjectName. We should just pass subjectName in the bundle.

@@ +105,5 @@
> +
> +  /**
> +   * Determines the identity mode corresponding to the icon we show in the urlbar.
> +   */
> +  getIdentityMode: function getIdentityMode(aState, uri) {

uri is unused

@@ +132,5 @@
> +
> +  getMixedActiveMode: function getActiveDisplayMode(aState) {
> +    // Only show an indicator for loaded mixed content if the pref to block it is enabled
> +    // TODO: Determine how mixed content will be handled in GV (should there be a setting, etc) 
> +    //       and handle as appropriate here.

I would just enable this because it's only indicating the current mode. It doesn't affect how mixed content is handled.

@@ +145,5 @@
> +
> +    return this.MIXED_MODE_UNKNOWN;
> +  },
> +
> +  getTrackingMode: function getTrackingMode(aState, aBrowser) {

aBrowser is unused

@@ +164,5 @@
> +   * (if available). Return the data needed to update the UI.
> +   */
> +  checkIdentity: function checkIdentity(aState, aBrowser) {
> +    let lastStatus = aBrowser.securityUI
> +                             .QueryInterface(Components.interfaces.nsISSLStatusProvider)

Ci.nsISSLStatusProvider

@@ +170,5 @@
> +
> +    // Don't pass in the actual location object, since it can cause us to
> +    // hold on to the window object too long.  Just pass in the fields we
> +    // care about. (bug 424829)
> +    let locationObj = {};

let lastLocation = {};

@@ +208,5 @@
> +    // mixed content is loaded (mixed display content is loaded by default).
> +    // We also return for CHROMEUI pages since they don't have any certificate
> +    // information to load either. result.secure specifically refers to connection
> +    // security, which is irrelevant for about: pages, as they're loaded locally.
> +    if (identityMode == this.IDENTITY_MODE_UNKNOWN ||

Use `===` and elsewhere

@@ +219,5 @@
> +    result.secure = true;
> +
> +    result.host = this.getEffectiveHost(lastLocation, uri);
> +
> +    let iData = this.getIdentityData(lastStatus);

Merge `getIdentityData` into this function, and pass along the raw cert data in the bundle instead of parsing information. E.g. pass `cert.organization` as "organization", `cert.subjectName` as "subjectName", etc.

@@ +221,5 @@
> +    result.host = this.getEffectiveHost(lastLocation, uri);
> +
> +    let iData = this.getIdentityData(lastStatus);
> +    //TODO: Decide how/if we want to expose verifier information in GV
> +    //result.verifier = Strings.browser.formatStringFromName("identity.identified.verifier", [iData.caOrg], 1);

Don't need this. The GV user should derive this string.

@@ +248,5 @@
> +      return result;
> +    }
> +
> +    // Cache the override service the first time we need to check it
> +    if (!this._overrideService)

Add braces

@@ +264,5 @@
> +        this._overrideService.hasMatchingOverride(lastLocation.hostname,
> +                                                  (lastLocation.port || 443),
> +                                                  iData.cert, {}, {})) {
> +      //TODO: Again, decide how/if we want to provide verifier information
> +      //result.verifier = Strings.browser.GetStringFromName("identity.identified.verified_by_you");

Don't need this.

@@ +265,5 @@
> +                                                  (lastLocation.port || 443),
> +                                                  iData.cert, {}, {})) {
> +      //TODO: Again, decide how/if we want to provide verifier information
> +      //result.verifier = Strings.browser.GetStringFromName("identity.identified.verified_by_you");
> +      result.securityException = true;

security_exception

@@ +273,5 @@
> +
> +  /**
> +   * Attempt to provide proper IDN treatment for host names
> +   */
> +  getEffectiveHost: function getEffectiveHost(lastLocation, uri) {

aLastLocation and aUri

@@ +274,5 @@
> +  /**
> +   * Attempt to provide proper IDN treatment for host names
> +   */
> +  getEffectiveHost: function getEffectiveHost(lastLocation, uri) {
> +    if (!this._IDNService)

Add braces

@@ +282,5 @@
> +      return this._IDNService.convertToDisplayIDN(uri.host, {});
> +    } catch (e) {
> +      // If something goes wrong (e.g. hostname is an IP address) just fail back
> +      // to the full domain.
> +      return this.lastLocation.hostname;

aLastLocation

@@ +338,5 @@
>    }
>  
>    onSecurityChange(aWebProgress, aRequest, aState) {
> +    // Don't need to do anything if the data we use to update the UI hasn't changed
> +    if (this._state == aState && !this._hostChanged)

`===` and Add braces

@@ +342,5 @@
> +    if (this._state == aState && !this._hostChanged)
> +      return;
> +
> +    this._state = aState;
> +    this._hostChanged = false;

`_hostChanged` is never set to true?

Comment 6

[Dylan Roeh (:droeh) (he/him)]

Attachment: Expose the bundle generated by IdentityHandler in ProgressListener v1.2

With recommended changes.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8885334 [details] [diff] [review]
Expose the bundle generated by IdentityHandler in ProgressListener v1.2

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

LGTM

::: mobile/android/modules/geckoview/GeckoViewProgress.jsm
@@ +177,5 @@
> +
> +    let status = lastStatus.QueryInterface(Ci.nsISSLStatus);
> +    let cert = status.serverCert;
> +
> +    result.subjectOrg = cert.organization;

I think it should just be `result.organization`, or maybe `result.subjectOrganization`. Update doc too.

@@ +250,5 @@
>      }
>    }
>  
> +  onSettingsUpdate() {
> +    debug("onSettingsUpdate: " + JSON.stringify(this.settings));

We should cache `this.settings` in a local variable because a new object is created every time `this.settings` is accessed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9212d856842
Expose a GeckoBundle containing site identity information to ProgressListener. r=jchen

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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