1241907 - Use "pm grant" to grant permissions before running browser tests

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Geoff Brown [:gbrown] (pto Apr 11-18)]

+++ This bug was initially created as a clone of Bug #1234799 +++

In bug 1234799, 'mach install' changed to grant all runtime permissions. That allows for cases like "mach install && mach robocop", but it also means that developers automatically get all permissions granted just by using mach to install. That might be confusing (why am I not being prompted for that permission?) or inconvenient (developers need to 'pm revoke' permissions to observe behavior without permissions being granted).

It might be better not to grant permissions at install time, but grant permissions at test time instead: Have the test harnesses, or perhaps mach, invoke "pm grant" before running tests. A complication is determining which permissions to grant; see https://bugzilla.mozilla.org/show_bug.cgi?id=1234799#c12.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

This fooled me several times today. I was editing C++ code to debug a permission issue and therefore was using mach commands instead of gradle. After running "mach install" all permission have been granted automatically and I had to manually revoke them again (and often forgot and had "confusing debug results").

Comment 2

[Geoff Brown [:gbrown] (pto Apr 11-18)]

I have work in progress and should have a patch ready soon.

Comment 3

[Geoff Brown [:gbrown] (pto Apr 11-18)]

It looks like "pm grant" requires a specific package and permission, so we need to issue something like:

  adb shell pm grant org.mozilla.fennec_$USER android.permission.WRITE_EXTERNAL_STORAGE
  adb shell pm grant org.mozilla.fennec_$USER android.permission.ACCESS_FINE_LOCATION
  adb shell pm grant org.mozilla.fennec_$USER android.permission.CAMERA
  adb shell pm grant org.mozilla.fennec_$USER android.permission.WRITE_CONTACTS

The package name needs to match the "app" argument used by the test harness. Only one permission can be granted at a time. On success, "pm grant" returns 0 and prints no message.


My test for verifying a grant: On an Android 6.0 device, open Fennec, load a page, select Menu > Page > Save as PDF; if android.permission.WRITE_EXTERNAL_STORAGE has not been granted, there is a runtime prompt "Allow <app> to access photos, media, and files on your device?"; if it has been granted, there is no prompt.

Comment 4

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: issue 'pm grant' from mach test commands

This removes the 'install -g' changes to 'mach install' added in bug 1234799 and replaces that functionality with 'pm grant' calls issued at test time.

grant_runtime_permissions() is added to android_device.py, alongside other utilities used by Android mach commands. It hard-codes the required permissions rather than trying to parse the AndroidManifest; that seems reasonable given that we expect the permissions not to change much over time -- https://bugzilla.mozilla.org/show_bug.cgi?id=1234799#c12.

grant_runtime_permissions() needs to be called in 3 places: prior to starting the browser for reftest, mochitest, and robocop tests. (Runtime permissions should not be necessary for xpcshell or cppunit tests, which don't start the browser.)

When running browser tests on an Android 6 device, mach should now report something like:
  Granting important runtime permissions to org.mozilla.fennec_gbrown

When running tests on a device running an earlier version of Android, no message is reported and no 'pm grant' calls are attempted.

This patch will not affect tests run from mozharness or autophone -- we'll keep using the 'install -g' approach in those environments since it is more efficient and avoids the risk of missing new permissions.

Comment 5

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

Comment on attachment 8714500 [details] [diff] [review]
issue 'pm grant' from mach test commands

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

is this all the mach_commands we need to edit?  Any chance we have to edit other code to make things run in emulator or autophone automation to match this?

Comment 6

[Geoff Brown [:gbrown] (pto Apr 11-18)]

I think I've covered all the cases. The mach test commands use the same code paths for emulator/device after verify_android_device. 'install -g' remains baked into autophone (via adb_android.py) so we need not grant permissions for autophone whether it is running in production or locally.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a6cf1133bb5

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a6cf1133bb5

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

The build errors (bug 1243469)
 error: device '(null)' not found
 /bin/sh: line 0: [: -gt: unary operator expected
are present in 46, now on beta. While they appear to be harmless, is this fix upliftable ?

Comment 11

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Sorry, didn't realize this was a concern!

Yes, it can be uplifted. The m-c patch even applies cleanly.