Closed Bug 487467 Opened 11 years ago Closed 11 years ago

Network Geolocation Provider

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dougt, Assigned: dougt)

References

Details

(Keywords: fixed1.9.1)

Attachments

(7 files, 3 obsolete files)

Using the base WIFI support in Necko, we can provider applications a simple way to enable a network geolocation provider.

Following patch implements a geolocation provider that can be pointed at any URI that will package up the Wifi data, and handle a response from the geolocation network provider.
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #371704 - Flags: superreview?
Attachment #371704 - Flags: review?(jst)
tracking-fennec: --- → ?
Flags: wanted1.9.1?
Comment on attachment 371704 [details] [diff] [review]
patch v.1

+WifiGeoCoordsObject.prototype = {
+
+    QueryInterface:   XPCOMUtils.generateQI([Ci.nsIDOMGeoPositionCoords, Ci.nsIClassInfo]),
+
+    getInterfaces: function(countRef) {
+        var interfaces = [Ci.nsIDOMGeoPositionCoords, Ci.nsIClassInfo, Ci.nsISupports];
+        countRef.value = interfaces.length;
+        return interfaces;
+    },
+
+    getHelperForLanguage: function(language) null,
+    contractID: "",
+    classDescription: "wifi geo position coords object",
+    classID: null,
+    implementationLanguage: Ci.nsIProgrammingLanguage.JAVASCRIPT,
+    flags: Ci.nsIClassInfo.DOM_OBJECT,

Do you need this flag here? This object isn't directly exposed to content code on a webpage is it?

+                try { accessToken = prefService.getCharPref("geo.wifi.access_token"); } catch (e) {}

Per discussion in the security review of this feature, this pref should be bound to the provider url so we don't leak access tokens from one provider to another.

r+sr=jst with that looked into.
Attachment #371704 - Flags: superreview?
Attachment #371704 - Flags: superreview+
Attachment #371704 - Flags: review?(jst)
Attachment #371704 - Flags: review+
Attached patch patch v.2 (obsolete) — Splinter Review
Thanks jst for the review.  so, the coords object needs to have that class info flag on it because we do pass it in to content.  I tried removing it, and i could not access the object off of the position object.

This patch also addresses your concern about ensuring that the hash only goes back to the same provider URI that set it.

I also have included a simple way to get back IP look ups for the LAN case.
Attachment #371704 - Attachment is obsolete: true
Attachment #371798 - Flags: superreview?(jst)
Attachment #371798 - Flags: review?(jst)
Attachment #371799 - Flags: superreview?(gavin.sharp)
Attachment #371799 - Flags: superreview?
Attachment #371799 - Flags: review?(gavin.sharp)
Attachment #371799 - Flags: review?
Attachment #371869 - Flags: superreview?(gavin.sharp)
Attachment #371869 - Flags: superreview?
Attachment #371869 - Flags: review?(gavin.sharp)
Attachment #371869 - Flags: review?
Attached patch mochitestSplinter Review
mochitest patch which ensures that we always talk to the correct "testing" geolocation provider.
Attached patch google url (obsolete) — Splinter Review
this patch will point the browser at google's geolocation service.
Attachment #371876 - Flags: superreview?
Attachment #371876 - Flags: review?(beltzner)
Attachment #371876 - Flags: superreview? → superreview?(shaver)
Comment on attachment 371798 [details] [diff] [review]
patch v.2

>diff --git a/dom/src/geolocation/NetworkGeolocationProvider.js b/dom/src/geolocation/NetworkGeolocationProvider.js

>+function nowInSeconds()

This is equivalent to Date.now()/1000;

>+function LOG(aMsg) {
>+    aMsg = ("*** WIFI GEO: " + aMsg);
>+    Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).logStringMessage(aMsg);

This seems like it may be pretty spammy... could it be pref-based, or only on in debug builds, or something?

>+                var accessTokenPrefName = "geo.wifi.access_token." + req.target.channel.URI.spec;

Are you sure all possible characters in that URI are pref-friendly? Should this be using the prePath instead?
Comment on attachment 371874 [details] [diff] [review]
mochitest

r=ctalbert

It's a good workaround until we figure out if we can just seed the mochitest profile with the test geolocation plugin rather than the standard geolocation plugin.
Attachment #371874 - Flags: review+
Comment on attachment 371798 [details] [diff] [review]
patch v.2

Would also be nicer to just pass a JS object to JSON.stringify() rather than building the JSON string manually.
Comment on attachment 371799 [details] [diff] [review]
patch to browser to remove prefs when someone clears sessions

>diff --git a/browser/base/content/sanitize.js b/browser/base/content/sanitize.js

var branch = Components.classes["@mozilla.org/preferences-service;1"]
                       .getService(Components.interfaces.nsIPrefBranch);
try {
  branch.deleteBranch("geo.wifi.access_token.");
} catch (ex) {}

Should work, and would remove the need to instantiate a temp branch object (not that it matters too much).

I think this should be cleared as part of cookies, not "active logins".
Attachment #371799 - Flags: superreview?(gavin.sharp)
Attachment #371799 - Flags: review?(gavin.sharp)
Attachment #371799 - Flags: review+
Comment on attachment 371869 [details] [diff] [review]
patch to browser to remove prefs when someone clears sessions (Sanitizer.jsm)

This is dead code atm, but you might as well sync it up too (one day we'll fix bug 397719). Same comments apply.
Attachment #371869 - Flags: superreview?(gavin.sharp)
Attachment #371869 - Flags: review?(gavin.sharp)
Attachment #371869 - Flags: review+
Attached patch patch v.3Splinter Review
incorporates gavin's suggestions.
Attachment #371798 - Attachment is obsolete: true
Attachment #371798 - Flags: superreview?(jst)
Attachment #371798 - Flags: review?(jst)
Attachment #371902 - Flags: superreview?
Attachment #371902 - Flags: review?
Attachment #371902 - Flags: review? → review+
Comment on attachment 371902 [details] [diff] [review]
patch v.3

>diff --git a/dom/src/geolocation/Makefile.in b/dom/src/geolocation/Makefile.in

>+EXTRA_COMPONENTS = \
>+                 $(srcdir)/NetworkGeolocationProvider.js \
>+                 $(NULL)

That $(srcdir) shouldn't be necessary, right?

>diff --git a/dom/src/geolocation/NetworkGeolocationProvider.js b/dom/src/geolocation/NetworkGeolocationProvider.js

>+function getAccessTokenForURL(url)

>+        var accessTokenPrefName = "geo.wifi.access_token." + url;

This still concerns me a bit, but maybe that concern isn't warranted. I guess the pref API just sees raw bytes (for better or worse).

>+    onChange: function(accessPoints) {

>+        // set something so that we can strip cookies
>+        xhr.channel.loadFlags = Ci.nsIChannel.LOAD_ANONYMOUS;

I suppose you'll probably want to set .mozBackgroundRequest as well, to avoid bad-cert error dialogs (if the URL happens to be SSL). Though that brings up the question of whether we should be using SSL for these requests anyways... bigger burden on providers, but sending lists of nearby wifi access points without that protection seems kind of risky.

>+        xhr.onerror = function(req) {

>+        xhr.onload = function (req) {  

I recall bz mentioning that some part of XHR was broken for components (related to createInstance vs. new XMLHttpRequest)... maybe ask him about it? Or maybe jst knows.

>+        var wifi_towers = new Array();
>+        for (var i=0; i<accessPoints.length; i++) {
>+            var tower = {};
>+            
>+            tower.mac_address = accessPoints[i].mac;
>+            tower.ssid = accessPoints[i].ssid;
>+            tower.signal_strength = accessPoints[i].signal;
>+            wifi_towers[i] = tower;
>+        }
>+
>+        request.wifi_towers = wifi_towers;

All this can just be:

request.wifi_towers = acessPoints.map(function (ap) ({
  mac_address: ap.mac,
  ssid: ap.ssid,
  signal_strength: ap.signal,
}));

Re-inventing cookies here kind of bothers me in general, particularly for the exposability aspect (people won't necessarily know about these pref values, but do generally know about cookies), but I wasn't at the security review for this feature, so I think I'm probably missing some context that would be useful in reviewing this. Dolske mentions that one of the suggestions there was to also avoid sending the accessToken if cookies were disabled, which seems like a good idea and should be easy enough (add a pref check in getAccessTokenForURL?).
we aren't reinventing cookies here.  gls is used by handsets that do not support http cookies (in j2me). plus, you dont want cookies to be forwarded when to go to the provider's url in the browser.

Using SSL is right for privacy.  I do not want my location in the clear.  I will add the mozBackgroundRequest flag though.

I am not sure what to do about your concern about XHR here.  Is there a related bug number?
Attachment #371902 - Flags: superreview? → superreview?(jst)
Attachment #371902 - Flags: superreview?(jst) → superreview+
(In reply to comment #16)
> we aren't reinventing cookies here.

This token behaves almost identically to a cookie, and my concern is that people who care about such things won't notice this separate way of being uniquely identified. I don't have any better ideas, though, and obeying cookie prefs and cookie-clearing mitigates that concern significantly. 

> Using SSL is right for privacy.

Doh! I misread the URL patch, I didn't notice that it was already SSL. Nevermind!

> I am not sure what to do about your concern about XHR here.  Is there a related
> bug number?

I can't find one. Don't worry about it, I'll ask bz and can file a followup if there's a problem. I don't see a problem with the current patch.
Comment on attachment 371876 [details] [diff] [review]
google url

>+// base url for the wifi geolocation network provider
>+pref("geo.wifi.uri", "chrome://browser-region/locale/region.properties");

For now, let's just make this 

pref("geo.wifi.uri", "https://www.google.com/loc/json");

which addresses the concern of ensuring that the location lookup service is preference-settable.

Then let's only add this part:

>+# base url for the wifi geolocation network provider
>+geo.wifi.uri=https://www.google.com/loc/json

after b4 so that we don't require l10n support on these strings until after that milestone.

Further, Axel wonders if this is something we want to make locale specific, as users travelling with their laptops might be better served by a local-to-their-physical location provider.
Attachment #371876 - Flags: superreview?(shaver)
Attachment #371876 - Flags: review?(beltzner)
Attachment #371876 - Flags: review-
Note, if we actually come to the conclusion that we really need to make this a localized pref, we could still do that as an optional entity, but that requires a tad more work to adjust the exception rules.

For example, we could make a localized pref geo.wifi.l10n.uri, and fall back to geo.wifi.uri if that doesn't exist, and only add that value to the locale that actually needs customization.

Adding an entry to region.properties comes with a lot of adminstrative overhead, so unless there's a real world value in doing so, I'd rather not add an entry at all.
Axel, suppose that in the future in some region there is a better service to use, and that we worked with that provider and they offer the same security, privacy, etc that the current provider offers.  Futher suppose that the region wanted to use that instead of what is used in the rest of the world.  

Would it be better to change the global preference for that region or change it in region.properties?
Attached patch google url v.2Splinter Review
Attachment #371876 - Attachment is obsolete: true
Attachment #372635 - Flags: review?(beltzner)
Attachment #372635 - Flags: review?(beltzner) → review+
I'm not sure if adding the provider to the localization is the right way to offer that service to users. Like, if the service is good, why shouldn't a user of another localization traveling through the region in question benefit? We did a few experiments with location-based services (maps in thunderbird), and those never really worked out well being "one service per locale".

IMHO, we should figure out how to add more providers once we have more than one, and a concrete rationale to take it. Then it'll probably be more than apparent on how to implement it.
axel, I have plans to support multiple providers (bug 454490).  Until then, does attachment 372635 [details] [diff] [review] work for you?  And, if for some reason, we wanted needed to change this preference in a particular locale, would it be possible?
Yes, we could add the pref to firefox-l10n.js, that overrides firefox.js, IIRC.
Comment on attachment 372635 [details] [diff] [review]
google url v.2

r=me as a temporary measure for b4, but we'll want this in region.properties in the future (either post-b4, or at least on trunk).
Attachment #372635 - Flags: review+
Sorry, I actually hadn't read Axel's comments. I retract my comment about region.properties :)
Attached patch complete patchSplinter Review
Blocks: FF2SM
Blocks: 488472
No longer blocks: FF2SM
when switching over to not use the localization preference, we didn't change the call into the pref services.
Attachment #372879 - Flags: review?
Attachment #372879 - Flags: review? → review+
Comment on attachment 372879 [details] [diff] [review]
fix to localization pref

a191=beltzner
Blocks: 491009
clearing 1.9.1 request flag.  this was fixed for 1.9.1
Flags: wanted1.9.1?
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.