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

Remove all location permissions from sdk manifest #430

Merged
merged 2 commits into from
Oct 30, 2019

Conversation

nkukday
Copy link
Contributor

@nkukday nkukday commented Oct 16, 2019

Problem
For apps targeting <= Android 9 but running on Android 10, the system automatically adds ACCESS_BACKGROUND_LOCATION when updating to Android 10 if either of ACCESS_FINE_LOCATION or ACCESS_COARSE_LOCATION is declared in the manifest.

Solution
Removing all location permissions from SDK to prevent ACCESS_BACKGROUND_LOCATION being added to app's manifest as a result of inheriting location permissions from SDK.

Test
Build Navigation and Maps successfully.

@nkukday nkukday self-assigned this Oct 16, 2019
@langsmith
Copy link
Contributor

cc @mapbox/maps-android

@zugaldia
Copy link
Member

Instead of removing all location permissions for all API levels, have you considered setting the value of android:maxSdkVersion within uses-permission so that it only gets included in pre-10 versions?

@alfwatt
Copy link
Contributor

alfwatt commented Oct 22, 2019

@zugaldia see @nkukday's comments: https://github.com/mapbox/mobile-telemetry/issues/445#issuecomment-544612609

Looks like adding the maxSKDVersion complicates things for apps which include the SDK, is there anything else blocking approval on this change?

@langsmith
Copy link
Contributor

For what it's worth:

I just built https://github.com/mapbox/mapbox-gl-native-android perfectly fine with 4.7.0-SNAPSHOT for libtelemetry and 1.4.0-SNAPSHOT for libcore: https://github.com/mapbox/mapbox-gl-native-android/blob/master/gradle/dependencies.gradle#L11-L12

Was able to install the /mapbox-gl-native-android test app on my phone and ran a number of the examples. Everything looked fine.

@tobrun
Copy link
Member

tobrun commented Oct 23, 2019

From https://github.com/mapbox/mobile-telemetry/issues/445#issuecomment-544612609:

The host app will inherit android:maxSdk="28" from us so they will have to explicitly remove this permission and then add their own or they will lose their ACCESS_FINE_LOCATION past API 28.

great catch, this enforces configurations on users that they don't expect, 👍 on removal

Copy link
Contributor

@andrlee andrlee left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #430 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master    #430      +/-   ##
===========================================
- Coverage     68.25%   68.2%   -0.05%     
  Complexity      382     382              
===========================================
  Files            68      68              
  Lines          2079    2079              
  Branches        163     163              
===========================================
- Hits           1419    1418       -1     
- Misses          570     571       +1     
  Partials         90      90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants