Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use "navigator.permissions" to detect geolocation support #3571

Merged
merged 2 commits into from
Nov 10, 2016

Conversation

tmcw
Copy link
Contributor

@tmcw tmcw commented Nov 8, 2016

Due to new security restrictions, the presence of navigator.geolocation is no longer a complete indication of geolocation support: the navigator.permissions API allows one to query for access to the API. This PR moves geolocation support checking to the GeolocationControl in order to support this asynchronous API.

Testing plan:

  1. npm run start-debug
  2. edit /etc/hosts to give your localhost another creative name, since the Chrome HTTPS rule whitelists localhost
  3. confirm that the control is not visible on the alternative name, but is visible on localhost

Implementation notes:

Since the permissions API is async and is not instant, this approach caches the result. I have not found any cases where the result will change.

Launch Checklist

Fixes #3568 - waiting on #3497 because this needs to move the geolocation detection into the geocodercontrol itself. Actionable - workingonit.

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@mourner
Copy link
Member

mourner commented Nov 9, 2016

we should move the geolocation detection to the control
itself

Yep! This will allow us to consistently disable geolocation if it's not supported, no matter at what time the geolocation control is added.

@tmcw tmcw force-pushed the detect-insecurity branch from 6e5ee70 to d1197fe Compare November 9, 2016 20:14
@tmcw tmcw changed the title [wip] Detect insecurity Detect insecurity Nov 9, 2016
Due to new [security restrictions](https://sites.google.com/a/chromium.org/dev/Home/chromium-security/deprecating-powerful-features-on-insecure-origins), the presence of `navigator.geolocation` is no longer a complete indication of geolocation support: the `navigator.permissions` API allows one to query for access to the API. This PR moves geolocation support checking to the GeolocationControl in order to support this asynchronous API.
@tmcw tmcw force-pushed the detect-insecurity branch from b4a8ef1 to 305403f Compare November 9, 2016 20:41
@tmcw
Copy link
Contributor Author

tmcw commented Nov 9, 2016

This is now ready for review @mourner @lucaswoj

callback();
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be slightly cleaner to call the callback in any outcome, and move responsibility for checking supportsGeolocation value to _setupUI:

function checkGeolocationSupport(callback) {
    if (supportsGeolocation !== undefined) {
        callback();

    } else if (window.navigator.permissions !== undefined) {
        window.navigator.permissions.query({name: 'geolocation'}).then((p) => {
            supportsGeolocation = p.state !== 'denied';
            callback();
        });
    } else {
        supportsGeolocation = !!window.navigator.geolocation;
        callback();
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already tried both variations, but I'm flexible. I'll switch it to the suggested style.

* browsers including Chrome requires sites to be served over HTTPS. If
* geolocation support is not available, the GeolocateControl will not
* be visible.
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, should we limit the line width to 70 chars in comments like this? IMHO increasing the limit would help readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how you're set up, but in vim-land I never use wrapping so cutting lines like this is more readable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use wrapping too, it just that 70 seems too short, given that we use up to 120 for the code.

@lucaswoj lucaswoj changed the title Detect insecurity Use "navigator.permissions" to detect geolocation support Nov 10, 2016
@tmcw tmcw merged commit 1926aa5 into master Nov 10, 2016
@tmcw tmcw deleted the detect-insecurity branch November 10, 2016 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants