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

RESOLVED FIXED in Firefox 47

Status

()

Firefox for Android
Testing
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 47
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
+++ 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.
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").
Blocks: 1239284
(Assignee)

Comment 2

3 years ago
I have work in progress and should have a patch ready soon.
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8714500 [details] [diff] [review]
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.
Attachment #8714500 - Flags: review?(jmaher)
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?
Attachment #8714500 - Flags: review?(jmaher) → review+
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1243469

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a6cf1133bb5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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 ?
Flags: needinfo?(gbrown)
(Assignee)

Comment 11

2 years ago
Sorry, didn't realize this was a concern!

Yes, it can be uplifted. The m-c patch even applies cleanly.
Flags: needinfo?(gbrown)
(Assignee)

Updated

a year ago
Depends on: 1340175
You need to log in before you can comment on or make changes to this bug.