882485 - Add API keys support for Google Location Service API.

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Doug Turner (:dougt)]

+++ This bug was initially created as a clone of Bug #835890 +++

Folks at Google have asked us to investigate using specific API keys for making requests to Google APIs from Firefox release builds. The goal is to allow them to put further restrictions on non-Firefox uses of those APIs.

Some preliminary information about how this works in Chrome:
> This is the link for contributors outside of Google:
> http://www.chromium.org/developers/how-tos/api-keys
> 
> The key implementation files in the Chromium source tree are:
> 
> a) A central point for retrieving the API key and OAuth2 tokens that
> are set on the build. This picks up the key and tokens that are baked
> in via preprocessor defines or in an internal-only header file, and
> bakes them into the binary. It also allows overriding values at
> runtime via environment variables and command-line switches. See
> http://code.google.com/p/chromium/source/search?q=google_api_keys.h
> and its associated .cc file.
> 
> b) A build configuration that detects whether the
> google_chrome_api_keys.h file is available and uses it automatically
> if it is, depending on configuration. See
> http://code.google.com/p/chromium/source/search?q=common%5C.gypi+google_api
> (top result) and a Python script it depends on,
> http://code.google.com/p/chromium/source/search?q=build/check_internal%5C.py



This is a requirement of the current GLS contract -- without sending keys, we can not do Geolocation.

Comment 1

[Doug Turner (:dougt)]

Attachment: tim's patch

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

jdm:

please review the geo specific changes. This is a good change since we do longer have to deal with the access token.  I took this opportunity to remove support for access token and private browsing mode.

https://tbpl.mozilla.org/?tree=Try&rev=eb0f5048daa9

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 761890 [details] [diff] [review]
patch v.1

gps:

Would love a build peer review!  The build changes are basically based on Tim's changes.


brendan: we are adding support to add a Google API key to the binary.  This will allow us to use Google services that now require API Keys.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 761890 [details] [diff] [review]
patch v.1

gavin:

Please review the browser pieces.  Most of this is removal of code that is no longer required.  The older api had an access token that needed to be cleared when the user 'cleared data'.  We no longer need to do this.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 761890 [details] [diff] [review]
patch v.1

The browser/ changes (really the entire patch) look fine to me.

Gerv may be a better person to sign off on the "include a secret" concept.

Should we keep the requestPrivate flag and use it to set the XHR's notificationCallbacks to an nsILoadContext whose usePrivateBrowsing getter returns true? Might not matter in practice (LOAD_ANONYMOUS should be roughly equivalent?) but it seems "safer".

Comment 6

[Doug Turner (:dougt)]

> Should we keep the requestPrivate flag

I'll let jdm comment about this.  I don't know, but hope that LOAD_ANONYMOUS is enough.  If it isn't :(

Comment 7

[Hanno Schlichting]

One nice thing we could add: Filter out wifi's with an SSID ending in _nomap on the client side. This is the Google standard for "wifi owner opt-out" and they should apply it on the server side. But best to not sent them data in the first place.

Comment 8

[Doug Turner (:dougt)]

hanno +1

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 761890 [details] [diff] [review]
patch v.1

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

Granting r+ despite bit rot. I trust you can handle it.

::: configure.in
@@ +4358,5 @@
>  
> +# Allow to specify a Google API key file that contains the secret key to be
> +# used for various Google API requests.
> +MOZ_ARG_WITH_STRING(google-api-keyfile,
> +[  --with-google-api-keyfile=file   Use the secret key contained in the given keyfile for Google AP requests],

API

::: toolkit/components/urlformatter/Makefile.in
@@ +15,5 @@
>    $(NULL)
>  
> +EXTRA_PP_COMPONENTS = \
> + nsURLFormatter.js \
> + $(NULL)

You got bit rotted by bug 880246.

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 761890 [details] [diff] [review]
patch v.1

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

I'm pleased. I don't think using a private channel actually gives us any benefit here, since the request isn't exposing any data about the user's browsing habits, so LOAD_ANONYMOUS should be fine.

::: dom/system/NetworkGeolocationProvider.js
@@ +144,5 @@
>        return b.signal - a.signal;
>      };
>  
>      function encode(ap) {
> +      return "{'macAddress':'" + ap.mac + "', 'signalStrength':'" + ap.signal + "'}"; 

I'd prefer a JSON.stringify call on the literal object instead.

@@ +152,2 @@
>      if (accessPoints) {
> +        data = "{'wifiAccessPoints': [" + accessPoints.sort(sort).map(encode).join(",") + "]}"

Actually, a stringify call here would be even better. Just make encode return the JS object.

@@ +159,5 @@
>      let xhr = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]
>                          .createInstance(Ci.nsIXMLHttpRequest);
>  
>      // This is a background load
> +  

Whitespace.

@@ +169,5 @@
> +    xhr.onerror = function() {
> +        LOG("onerror: " + xhr);
> +    };
> +
> +    xhr.onload = function() {  

Whitespace.

@@ +182,5 @@
>  
> +        let newLocation = new WifiGeoPositionObject(xhr.response.location.lat,
> +                                                    xhr.response.location.lng,
> +                                                    xhr.response.accuracy);
> +        

Whitespace.

Comment 11

[Josh Matthews [:jdm]]

Thank you splinter for totally mucking up the previous review+ flags.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 761890 [details] [diff] [review]
patch v.1

So long as we aren't obliged to hide (as in obfuscate, if not encrypt using some TPM we don't require) the key somehow, this is fine.

/be

Comment 13

[Gervase Markham [:gerv]]

I agree with Brendan, although I think this is worth having a community discussion about, so we can help people understand why. dougt said that there's a need to move quickly; does anyone have any concrete deadlines related to this change?

I'm away on holiday all next week, so I can't lead it :-|

Does this require a keyfile to be deployed to all our builders? I assume the key isn't checked into our public source repo...?

Gerv

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to Gervase Markham [:gerv] from comment #13)
> Does this require a keyfile to be deployed to all our builders? I assume the
> key isn't checked into our public source repo...?

Right. Rel-eng will need to figure out how to make this work.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 761890 [details] [diff] [review]
patch v.1

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

::: configure.in
@@ +4359,5 @@
> +# Allow to specify a Google API key file that contains the secret key to be
> +# used for various Google API requests.
> +MOZ_ARG_WITH_STRING(google-api-keyfile,
> +[  --with-google-api-keyfile=file   Use the secret key contained in the given keyfile for Google AP requests],
> +  MOZ_GOOGLE_API_KEY=`cat $withval`)

I'd prefer if you just did like
MOZ_GOOGLE_API_KEY_FILE=$withval)
if test -n "$MOZ_GOOGLE_API_KEY_FILE" -a ! -f "$MOZ_GOOGLE_API_KEY_FILE"; then
  AC_MSG_ERROR([The key file $MOZ_GOOGLE_API_KEY_FILE does not exist.])
fi
MOZ_GOOGLE_API_KEY=`cat "$MOZ_GOOGLE_API_KEY_FILE"`

So you get a more useful error message if you screw this up. (In fact your current patch won't actually break the build if the file is misnamed or missing!)

Comment 16

[[e-mail] (:bhearsum)]

This approach looks fine for RelEng. I'm not quite sure where we'll store them or how exactly we'll get them onto the machines at build time, but that doesn't affect this. I filed bug 883233 for the RelEng parts, with some open questions.

Comment 17

[Doug Turner (:dougt)]

> I agree with Brendan, although I think this is worth having a community discussion about, so we can help people understand why.

I started a post on dev.planning.

Comment 18

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c236b24064b

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6c236b24064b

Comment 21

[(not currently active) Ted Mielczarek]

(In reply to Ted Mielczarek [:ted.mielczarek] (away June 25th-30th) from comment #15)
> So you get a more useful error message if you screw this up. (In fact your
> current patch won't actually break the build if the file is misnamed or
> missing!)

You didn't actually fix this, so this is kind of awful, in that it's really hard to tell if you got the option right.

Comment 22

[Masatoshi Kimura [:emk]]

Doug, you moved mozBackgroundRequest after xhr.open(). It caused test failures in bug 1132347. Note that mozBackgroundRequest after xhr.open() has never had effect from the start. Bug 1132347 just made it explicit.
Why did you move it? Is mozBackgroundRequest needed here?

Comment 23

[Doug Turner (:dougt)]

The change I made was for aesthetics.  It's probably a bug that setting mozBackgroundRequest after open() but before the end of js execution doesn't work.