Closed Bug 1336920 Opened 7 years ago Closed 7 years ago

Display the presence/absence of a valid Google API key in about:support

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: francois, Assigned: aryx)

References

Details

Attachments

(1 file)

In order to prevent bugs like bug 1333516, we should make it easy to detect the presence or absence of the Google API key in a build of Firefox.

I suggest adding a line in about:support under Application Basics:

  Google API Key: enabled / disabled

This will be useful for geolocation and also for Safe Browsing V4.
IIRC the API key is always set, it just uses an invalid build default of "no-google-api-key". So you would want to check whether or not the API key is set and set to something other than this build default.

If you also want to display the status of the badly named MOZILLA_API_KEY, it uses a default of "no-mozilla-api-key". While displaying that key, I'd use "MLS API Key" instead, where MLS is the official abbreviation for "Mozilla Location Service". MLS is used by all release channels for determining the users country, which again influences the default search providers. MLS is also currently used by the aurora and nightly channels to offer geolocation and in Fennec to provide one of the opt-in "data gathering" options to contribute back data to MLS.

My 2cents are: If someone spends time on displaying the status of one of them, I'd go ahead and display the status of both of them.
Assignee: nobody → aryx.bugmail
Status: NEW → ASSIGNED
(In reply to Hanno Schlichting [:hannosch] from comment #1)
> IIRC the API key is always set, it just uses an invalid build default of
> "no-google-api-key". So you would want to check whether or not the API key
> is set and set to something other than this build default.

You're right. That's exactly what I had in mind:

  gapi == "no-google-api-key" => "disabled"
  (gapi != "") and (gapi != "no-google-api-key") => "enabled"

> My 2cents are: If someone spends time on displaying the status of one of
> them, I'd go ahead and display the status of both of them.

Sounds like a good idea.
Comment on attachment 8834410 [details]
Bug 1336920 - Display the presence/absence of a valid Google and Mozilla API key in about:support.

https://reviewboard.mozilla.org/r/110374/#review111574

::: toolkit/content/aboutSupport.js:40
(Diff revision 1)
>  // snapshot data.  Each function is passed its property's corresponding data,
>  // and it's the function's job to update the page with it.
>  var snapshotFormatters = {
>  
>    application: function application(data) {
> +    let strings = stringBundle();

stringBundle got called several times in this method, so I replaced these calls with one like in the other methods here.

::: toolkit/locales/en-US/chrome/global/aboutSupport.dtd:69
(Diff revision 1)
>  <!ENTITY aboutSupport.appBasicsProfiles "Profiles">
>  
>  <!ENTITY aboutSupport.appBasicsMultiProcessSupport "Multiprocess Windows">
>  
> +<!ENTITY aboutSupport.appBasicsKeyGoogle "Google Key">
> +<!ENTITY aboutSupport.appBasicsKeyMozilla "Mozilla Key">

"MLS" would be too cryptic for most people and Mozilla Location Services too long, imho. The generic Mozilla key could be used for more in the future.

::: toolkit/modules/Troubleshoot.jsm:233
(Diff revision 1)
>        data.autoStartStatus = e10sStatus.data;
>      } catch (e) {
>        data.autoStartStatus = -1;
>      }
>  
> +    data.keyGoogleFound = /^[a-zA-Z0-9_\-]{39}$/

Is there a documented format for the Google API keys?
Comment on attachment 8834410 [details]
Bug 1336920 - Display the presence/absence of a valid Google and Mozilla API key in about:support.

https://reviewboard.mozilla.org/r/110374/#review111580

::: toolkit/locales/en-US/chrome/global/aboutSupport.dtd:69
(Diff revision 1)
>  <!ENTITY aboutSupport.appBasicsProfiles "Profiles">
>  
>  <!ENTITY aboutSupport.appBasicsMultiProcessSupport "Multiprocess Windows">
>  
> +<!ENTITY aboutSupport.appBasicsKeyGoogle "Google Key">
> +<!ENTITY aboutSupport.appBasicsKeyMozilla "Mozilla Key">

I personally don't think that it matters that most people won't known what MLS stands for. There's a lot of information on that page that's only useful if you know what to look for and what it means.

::: toolkit/modules/Troubleshoot.jsm:233
(Diff revision 1)
>        data.autoStartStatus = e10sStatus.data;
>      } catch (e) {
>        data.autoStartStatus = -1;
>      }
>  
> +    data.keyGoogleFound = /^[a-zA-Z0-9_\-]{39}$/

There isn't a specific format.

That's why we should look for `"no-google-api-key"` and `"no-mozilla-api-key"` and if the key is not equal to those (or empty) then we consider it valid, as I suggested in comment 2.
Attachment #8834410 - Flags: review-
Comment on attachment 8834410 [details]
Bug 1336920 - Display the presence/absence of a valid Google and Mozilla API key in about:support.

https://reviewboard.mozilla.org/r/110374/#review111974

Looks reasonable overall; please request review again once you have addressed François' comment about Troubleshoot.jsm.

::: browser/base/content/test/general/browser_aboutSupport.js:15
(Diff revision 1)
> +  yield BrowserTestUtils.withNewTab({ gBrowser, url: "about:support" }, function* (browser) {
> +    let keyGoogleExpected =
> +      AppConstants.MOZILLA_OFFICIAL && AppConstants.RELEASE_OR_BETA ? "Yes" : "No";
> +    let keyGoogleActual = yield ContentTask.spawn(browser, null, function* () {
> +      let textBox = content.document.getElementById("key-google-box");
> +      yield ContentTaskUtils.waitForCondition(() => textBox.textContent,

Are you sure textBox.textContent won't be a string containing whitespace before we display the value?

Is there no event we could listen to, to know about:support is ready?

::: toolkit/locales/en-US/chrome/global/aboutSupport.dtd:69
(Diff revision 1)
>  <!ENTITY aboutSupport.appBasicsProfiles "Profiles">
>  
>  <!ENTITY aboutSupport.appBasicsMultiProcessSupport "Multiprocess Windows">
>  
> +<!ENTITY aboutSupport.appBasicsKeyGoogle "Google Key">
> +<!ENTITY aboutSupport.appBasicsKeyMozilla "Mozilla Key">

"Mozilla Location Service" is shorter than the "Registered Service Workers" string we already have in the same column, so I think it's fine.

I find it strange that we display "Yes"/"No" after a name that says 'Key' (I would expect to see the actual key there).
Should we make the other string "Google APIs"?
Attachment #8834410 - Flags: review?(florian)
Comment on attachment 8834410 [details]
Bug 1336920 - Display the presence/absence of a valid Google and Mozilla API key in about:support.

https://reviewboard.mozilla.org/r/110374/#review111974

> Are you sure textBox.textContent won't be a string containing whitespace before we display the value?
> 
> Is there no event we could listen to, to know about:support is ready?

> Are you sure textBox.textContent won't be a string containing whitespace before we display the value?

Okay, .trim()ed it.

> Is there no event we could listen to, to know about:support is ready?

Option 1: Fire a custom event after we set the value and listen for that.
Option 2: Add a mutation observer on the MLS key box and fire that whole function as callback.
Other ideas?

> "Mozilla Location Service" is shorter than the "Registered Service Workers" string we already have in the same column, so I think it's fine.
> 
> I find it strange that we display "Yes"/"No" after a name that says 'Key' (I would expect to see the actual key there).
> Should we make the other string "Google APIs"?

>I find it strange that we display "Yes"/"No" after a name that says 'Key' (I would expect to see the actual key there).

Would be "Google Key Found?" / "Mozilla Location Service Key Found?" and "Yes/No" be okay?
Comment on attachment 8834410 [details]
Bug 1336920 - Display the presence/absence of a valid Google and Mozilla API key in about:support.

https://reviewboard.mozilla.org/r/110374/#review113278

r+ because I think it's good enough and I don't want to bikeshed on this, but I still have some concerns:
- The new strings are OK to me, but I'm not sure they fit with the other strings in about:support (no other string there has a question mark at the end).
- The browser_aboutSupport.js test is mixing a test for whether we have the keys, and for whether we are displaying that information correctly in about:support. I think it would be cleaner to have the test for whether the keys are present be an xpcshell test, or be part of browser_Troubleshoot.js. And then focus the browser_aboutSupport.js test on verifying that we display correctly the data from Troubleshoot.jsm. But I'm not convinced this is worth fixing, so r+ anyway.
Attachment #8834410 - Flags: review?(florian) → review+
Comment on attachment 8834410 [details]
Bug 1336920 - Display the presence/absence of a valid Google and Mozilla API key in about:support.

https://reviewboard.mozilla.org/r/110374/#review113878

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> r+ because I think it's good enough and I don't want to bikeshed on this,
> but I still have some concerns:

I share these concerns and I'd rather see another revision before we land this, sorry.

> - The new strings are OK to me, but I'm not sure they fit with the other
> strings in about:support (no other string there has a question mark at the
> end).

How about:

- "Google API Key" = "found" / "missing"
- "Mozilla Location Service Key": "found" / "missing" 

It's more consistent with the rest of the page and it conveys the same information.

> - The browser_aboutSupport.js test is mixing a test for whether we have the
> keys, and for whether we are displaying that information correctly in
> about:support.

The logic for whether or not the keys are available is quite complicated and currently the cause of some test failures. We should avoid testing for that here.

To avoid having this test because the source of intermittent failures, I suggest we simply test that the information is displayed and that the keys are either "found" or "missing".

::: browser/base/content/test/general/browser_aboutSupport.js:11
(Diff revision 2)
> +Cu.import("resource://gre/modules/AppConstants.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +add_task(function* () {
> +  yield BrowserTestUtils.withNewTab({ gBrowser, url: "about:support" }, function* (browser) {
> +    let keyGoogleExpected =

The logic is wrong here and is subject to change.

::: toolkit/modules/Troubleshoot.jsm:236
(Diff revision 2)
>      }
>  
> +    let keyGoogle = Services.urlFormatter.formatURL("%GOOGLE_API_KEY%").trim();
> +    data.keyGoogleFound = keyGoogle != "no-google-api-key" && keyGoogle.length > 0;
> +
> +    data.keyMozillaFound = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/

Can't we do the same thing as for the Google key here?

i.e. check for `no-mozilla-api-key`
Attachment #8834410 - Flags: review-
Comment on attachment 8834410 [details]
Bug 1336920 - Display the presence/absence of a valid Google and Mozilla API key in about:support.

https://reviewboard.mozilla.org/r/110374/#review114746

Thanks for revising this patch!
Attachment #8834410 - Flags: review+
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/6a1d6726edd0
Display the presence/absence of a valid Google and Mozilla API key in about:support. r=florian,francois
https://hg.mozilla.org/mozilla-central/rev/6a1d6726edd0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.