Closed
Bug 882485
Opened 11 years ago
Closed 11 years ago
Add API keys support for Google Location Service API.
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 24
People
(Reporter: dougt, Assigned: dougt)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jdm
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
+++ 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.
Assignee | ||
Updated•11 years ago
|
Group: mozilla-corporation-confidential
Assignee | ||
Comment 1•11 years ago
|
||
Assignee: nobody → doug.turner
Assignee | ||
Comment 2•11 years ago
|
||
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
Attachment #761890 -
Flags: review?(josh)
Assignee | ||
Comment 3•11 years ago
|
||
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.
Attachment #761890 -
Flags: superreview?(brendan)
Attachment #761890 -
Flags: review?(gps)
Assignee | ||
Comment 4•11 years ago
|
||
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.
Attachment #761890 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•11 years ago
|
Summary: investigate mechanism for using secret keys for Google API requests (safebrowsing, search suggest) → Add API keys support for Google Location Service API.
Assignee | ||
Updated•11 years ago
|
Attachment #761783 -
Attachment is obsolete: true
Comment 5•11 years ago
|
||
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".
Attachment #761890 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 6•11 years ago
|
||
> 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•11 years ago
|
||
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.
Assignee | ||
Comment 8•11 years ago
|
||
hanno +1
Comment 9•11 years ago
|
||
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.
Attachment #761890 -
Flags: review?(gps) → review+
Comment 10•11 years ago
|
||
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.
Attachment #761890 -
Flags: review?(josh)
Attachment #761890 -
Flags: review?
Attachment #761890 -
Flags: review+
Comment 11•11 years ago
|
||
Thank you splinter for totally mucking up the previous review+ flags.
Updated•11 years ago
|
Attachment #761890 -
Flags: review?
Comment 12•11 years ago
|
||
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
Attachment #761890 -
Flags: superreview?(brendan) → superreview+
Comment 13•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 17•11 years ago
|
||
> 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.
Assignee | ||
Comment 18•11 years ago
|
||
Comment 19•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Comment 21•11 years ago
|
||
(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•10 years ago
|
||
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?
Flags: needinfo?(dougt)
Updated•10 years ago
|
Assignee | ||
Comment 23•10 years ago
|
||
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.
Flags: needinfo?(dougt)
You need to log in
before you can comment on or make changes to this bug.
Description
•