1234799 - Granting permissions when running UI/robocop tests

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

With runtime permissions we might start to show dialogs here and there requesting access to things. These dialogs are created by the system and can't be controlled by UI testing frameworks.

An easy way out is to grant all permissions using adb and before running any tests:
> adb shell pm grant <package> android.permission.<permission>

Or install the APK with all runtime permissions granted automatically:
> adb install -g <apk>

This is not perfect because we will only test the happy path but it's better than no/broken tests for now.

For completeness, if we actually want to run a test without permissions:
> adb revoke pm grant <package> android.permission.<permission>

If we can grant permissions on a test-by-test basis (setUp()?) this gives us finer control. However for now it's probably easier to just install with -g.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

For try this is not important (for now): We do not run tests on Android 6 emulators in automation currently.

Comment 2

[Geoff Brown [:gbrown]]

Attachment: add -g to adb install issued by make and mach

This adds -g to the adb command used to install Firefox with "make install". "adb install -r <file>" is changed to "adb install -rg <file>". "mach install" invokes "make install" so this kills two birds with one tiny patch.

"adb install -g" is explicitly recognized by adb 1.0.32, which most people should be using. I also tested with adb 1.0.31, from sdk 18, and the install did not fail, so I expect this change will not adversely affect anyone.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8703815 [details] [diff] [review]
add -g to adb install issued by make and mach

Review of attachment 8703815 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see this -g option in my version .31 locally- testing doesn't seem to have issues locally- as this is for mach (not automation), I am fine moving forward with this.

Comment 4

[Geoff Brown [:gbrown]]

Attachment: add -g to adb install for emulator unit tests

This adds the -g option to adb install commands issued by android_emulator_unittest.py and androidx86_emulator_unittest.py. As noted in Comment 1, this should not be necessary, but it does no harm, and resolves a possible future issue for when/if we run tests on Android 6+.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fbf5104497f&selectedJob=15038362

Comment 5

[Geoff Brown [:gbrown]]

Leave open for autophone and perhaps other issues.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Does this mean every "mach install" will add the -g option? I'm not sure if we want that. For developers the app should be installed like normally and their builds should behave like a normal installation; e.g. crash if a permission has not been granted.

I fear that we might hide development errors if local builds always have all permissions granted magically.

Comment 7

[Geoff Brown [:gbrown]]

Yes, my intention was to add -g for every "mach install". 

"mach robocop|mochitest|..." runs tests against whatever is currently installed on the attached device. The test commands do not install Firefox, so something like "mach install && mach robocop" is not uncommon, and I'm thinking "mach install" is our easiest opportunity to grant permissions before running tests.

We could make -g a "mach install" option, but that seems like something that is easily forgotten.

Otherwise, we could "pm grant" in the mach test commands, or perhaps in the test harnesses, but that seems much more complicated.

Comment 8

[Geoff Brown [:gbrown]]

:sebastian -- Any more thoughts on "install -g" vs "pm grant", given comment 7?

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Geoff Brown [:gbrown] from comment #7)
> "mach robocop|mochitest|..." runs tests against whatever is currently
> installed on the attached device. The test commands do not install Firefox,
> so something like "mach install && mach robocop" is not uncommon, and I'm
> thinking "mach install" is our easiest opportunity to grant permissions
> before running tests.

Good point. I didn't think about "mach robocop" not installing the APK. I still fear that developers will miss that they need to check/request permissions in new code. However they'll need to be more careful in general now - and most of them use gradle anyways. So if landing this unblocks bug 1241267 then let's go ahead. I'll send a follow-up mail to mobile.firefox.dev afterwards so that everyone is aware of this.
Using "pm grant" in the test commands sounds like the best solution, maybe we can file a follow-up to explore this eventually.

Comment 10

[Geoff Brown [:gbrown]]

I had another look at using "pm grant". I think it would be easy enough to call "pm grant" from the test harnesses, but "pm grant" requires a list of permissions to be specified, and wildcards are not supported. So, which permissions would we specify? Would required permissions change over time?

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6beaef906da
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8e29521b09

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Geoff Brown [:gbrown] from comment #10)
> I had another look at using "pm grant". I think it would be easy enough to
> call "pm grant" from the test harnesses, but "pm grant" requires a list of
> permissions to be specified, and wildcards are not supported. So, which
> permissions would we specify?

Currently that's:
* android.permission.WRITE_EXTERNAL_STORAGE
* android.permission.ACCESS_FINE_LOCATION
* android.permission.CAMERA
* android.permission.WRITE_CONTACTS (This one is going away: bug 1193431, bug 1240598)

Our app has more permissions but only those are permissions with protection level "dangerous" - the ones we need to prompt for. All other permissions with protection level "normal" are automatically granted at install time.

(In reply to Geoff Brown [:gbrown] from comment #10)
> Would required permissions change over time?

Yes, but it's something that happens rarely. In the past permission bumps have been painful because they required *every* user to confirm the app update. Now with runtime permissions we may be able to do it more easily for Android 6.0 users but there's nothing planned in the near future as I can see it. Even then it would happen only rarely.

If we want to automate this then we could read the <uses-permission> tags [1] from the Manifest and call "pm grant" for those. You can't see which ones are dangerous in the manifest but I assume you can call "pm grant" on normal permissions too - even though this shouldn't have any effect.

This is the Manifest before preprocessing:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in

If we want to go this route then we should also be looking for <uses-permission-sdk-23> tags [2]. We don't use those yet but they'd allow us to define permissions only for Android 6.0 users -> No one has to grant this permission at install time but we can ask Android 6 users at runtime for it. This is becoming interesting for future features that would require a permission bump.

[1] http://developer.android.com/guide/topics/manifest/uses-permission-element.html
[2] http://developer.android.com/guide/topics/manifest/uses-permission-sdk-23-element.html

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d6beaef906da
https://hg.mozilla.org/mozilla-central/rev/fa8e29521b09