Bug 50566 – Delay starting geolocation request until permission has been granted or denied

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[John Knottenbelt]

If USE(PREEMPT_GEOLOCATION_PERMISSION) USE(PREEMPTIVE_GEOLOCATION), none of the steps to start a watch or get a position should be taken until permission has been set. This is in accordance with the specification:

"For both getCurrentPosition and watchPosition, the implementation must never invoke the successCallback without having first obtained permission from the user to share location. Furthermore, the implementation should always obtain the user's permission to share location before executing any of the getCurrentPosition or watchPosition steps described above." 
[ http://www.w3.org/TR/geolocation-API/#geolocation_interface ]

At present, the implementation does not meet the specification because the steps for checking for a suitable cached position, and zero timeout are taken before the permission has been set.

[John Knottenbelt]

Created attachment 75687 [details]
Patch

[Steve Block]

Comment on attachment 75687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75687&action=review

You're right that this change is required to meet the exact wording of the spec, though I'm not sure that this is intentionally the case! Is there particular motivation for this change?

See Bug 42068 for background for PREEMPT_GEOLOCATION_PERMISSION

> LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-timeout.js:1
> +description("Tests that when the timeout is 0, no callbacks are invoked until permission is denied. " +

Should probably name the test to reflect that it's specific to PREEMPT_GEOLOCATION_PERMISSION

> LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-timeout.js:3
> +            "USED(CLIENT_BASED_GEOLOCATION)");

Don't mention CLIENT_BASED_GEOLOCATION - it'll go away and we don't want to have to update the test.

> LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-timeout.js:28
> +    debug('error was ' + e.code + ':  ' + e.message);

Probably no need for this.

> LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-timeout.js:35
> +window.setTimeout(denyPermission, 100);

Do you need a delay of 100ms? It's best not to make tests rely on such magic numbers.

[John Knottenbelt]

Comment on attachment 75687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75687&action=review

>> LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-timeout.js:1
>> +description("Tests that when the timeout is 0, no callbacks are invoked until permission is denied. " +
> 
> Should probably name the test to reflect that it's specific to PREEMPT_GEOLOCATION_PERMISSION

Renamed to preempt-permission-timeout.html

>> LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-timeout.js:3
>> +            "USED(CLIENT_BASED_GEOLOCATION)");
> 
> Don't mention CLIENT_BASED_GEOLOCATION - it'll go away and we don't want to have to update the test.

Ok.

>> LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-timeout.js:28
>> +    debug('error was ' + e.code + ':  ' + e.message);
> 
> Probably no need for this.

Removed.

>> LayoutTests/fast/dom/Geolocation/script-tests/delayed-permission-timeout.js:35
>> +window.setTimeout(denyPermission, 100);
> 
> Do you need a delay of 100ms? It's best not to make tests rely on such magic numbers.

Timer was not important to the test. Removed. Thanks for the suggestion.

[John Knottenbelt]

The motivation for this change is primarily to make the code that starts the requests to be the same irrespective of whether it had to wait for permission to be set. 

For example, one difference, which is not easily testable with the current design of the Geolocation Mocks, is what happens if a page has two iframes, one of which is authorised for geolocation and has a watch started. If the second iframe requests a cached position, it should first get authorisation from the user, and then get the cached position that may have been stored in the Geolocation object thanks to the first iframe's request (if it meets the maxmium-age requirements).

There is also a discussion of some of these issues here: 

http://lists.w3.org/Archives/Public/public-geolocation/2009Mar/0068.html

[John Knottenbelt]

Created attachment 75699 [details]
Patch

[Steve Block]

> For example, one difference ...
I'm not exactly sure what you mean by difference. Currently, Geolocation does handle the situation you describe - the watch started in the second frame will make use of a cached position due to the first frame. Your patch does not change that - we will just ask for permission before, rather than after we check the position cache.

I'm still not 100% sure that the change in behaviour you're suggesting (check permission before looking for a cached position) is intended by the spec. The email discussion you reference talks about timeout, but not maximumAge. I'll discuss with Andrei.

[Steve Block]

The motivation for the 'should' for obtaining user permission before doing any work was that the location acquisition process may reveal sensitive information even if a position is not revealed to JavaScript. If permissions are not obtained preemtively ...
- If an API call results in a request for permission the app could deduce that a position was obtained and hence that a device is likely to be on WiFi - See https://bugs.webkit.org/show_bug.cgi?id=39434#c14
- Location acquisition using a server (eg WiFi ID look-up) could involve sending sensitive information over the network to a third party.

Having discussed with Andrei, the case of looking up a cached position wasn't really considered when the 'should' was written. The above concerns don't really apply to the case of looking up a cached position. In any case, we're still compliant with the spec as this is only a 'should'.

So unless the code clean-up you mention is important for other reasons, I'd suggest closing this bug as 'working as intended'.

[John Knottenbelt]

Thanks for considering it. This sounds reasonable to me. Let's close this one down.

(In reply to comment #7)
> The motivation for the 'should' for obtaining user permission before doing any work was that the location acquisition process may reveal sensitive information even if a position is not revealed to JavaScript. If permissions are not obtained preemtively ...
> - If an API call results in a request for permission the app could deduce that a position was obtained and hence that a device is likely to be on WiFi - See https://bugs.webkit.org/show_bug.cgi?id=39434#c14
> - Location acquisition using a server (eg WiFi ID look-up) could involve sending sensitive information over the network to a third party.
> 
> Having discussed with Andrei, the case of looking up a cached position wasn't really considered when the 'should' was written. The above concerns don't really apply to the case of looking up a cached position. In any case, we're still compliant with the spec as this is only a 'should'.
> 
> So unless the code clean-up you mention is important for other reasons, I'd suggest closing this bug as 'working as intended'.