Verify adb root took effect and add skip_root option

mobly/controllers/android_device.py

@@ -71,6 +71,11 @@
 # Default is False.
 KEY_SKIP_LOGCAT = 'skip_logcat'
 DEFAULT_VALUE_SKIP_LOGCAT = False
+# If True, `adb root` will not be run automatically on rootable (debuggable)
+# builds when the device is created and after a reboot handled via
+# `handle_reboot`. Default is False.
+KEY_SKIP_ROOT = 'skip_root'
+DEFAULT_VALUE_SKIP_ROOT = False
 SERVICE_NAME_LOGCAT = 'logcat'
 
 # Default name for bug reports taken without a specified test name.
@@ -326,8 +331,9 @@ def get_instances_with_configs(configs):
   for c in configs:
     serial = c.pop('serial')
     is_required = c.get(KEY_DEVICE_REQUIRED, True)
+    skip_root = c.pop(KEY_SKIP_ROOT, DEFAULT_VALUE_SKIP_ROOT)
     try:
-      ad = AndroidDevice(serial)
+      ad = AndroidDevice(serial, skip_root=skip_root)
       ad.load_config(c)
     except Exception:
       if is_required:
@@ -523,9 +529,12 @@ class AndroidDevice:
       via fastboot.
     services: ServiceManager, the manager of long-running services on the
       device.
+    skip_root: bool, if True, `adb root` is not run automatically on rootable
+      builds at creation time and after a reboot handled via `handle_reboot`.
+      Settable via the `skip_root` controller config key.
   """
 
-  def __init__(self, serial=''):
+  def __init__(self, serial='', skip_root=DEFAULT_VALUE_SKIP_ROOT):
     self._serial = str(serial)
     # logging.log_path only exists when this is used in an Mobly test run.
     _log_path_base = utils.abs_path(getattr(logging, 'log_path', '/tmp/logs'))
@@ -541,8 +550,8 @@ def __init__(self, serial=''):
     self._is_rebooting = False
     self.adb = adb.AdbProxy(serial)
     self.fastboot = fastboot.FastbootProxy(serial)
-    if self.is_rootable:
-      self.root_adb()
+    self.skip_root = skip_root
+    self._root_adb_if_allowed()
     self.services = service_manager.ServiceManager(self)
     self.services.register(
         SERVICE_NAME_LOGCAT, logcat.Logcat, start_service=False
@@ -753,8 +762,7 @@ def handle_reboot(
       self._build_info = None
       self._is_rootable = None
       self._is_rebooting = False
-      if self.is_rootable:
-        self.root_adb()
+      self._root_adb_if_allowed()
     self.services.start_services(live_service_names)
 
   @contextlib.contextmanager
@@ -932,11 +940,38 @@ def root_adb(self):
 
     If executed on a production build, adb will not be switched to root
     mode per security restrictions.
+
+    Note that `adb root` exits with code 0 even when adbd refuses to restart
+    as root (e.g. "adbd cannot run as root in production builds"), so the
+    outcome is verified by checking whether adb is actually running as root
+    afterwards instead of relying on the command's output.
+
+    Returns:
+      True if adb is running as root after this call, False otherwise.
     """
-    self.adb.root()
+    out = self.adb.root()
     # `root` causes the device to temporarily disappear from adb.
     # So we need to wait for the device to come back before proceeding.
     self.adb.wait_for_device(timeout=DEFAULT_TIMEOUT_BOOT_COMPLETION_SECOND)
+    if self.is_adb_root:
+      return True
+    self.log.warning(
+        '`adb root` did not take effect, continuing without root. Output: %s',
+        out,
+    )
+    return False
+
+  def _root_adb_if_allowed(self):
+    """Runs `root_adb` on rootable builds unless `skip_root` is set.
+
+    Called automatically at creation time and after a reboot handled via
+    `handle_reboot`.
+    """
+    if self.skip_root:
+      self.log.debug('Skipping automatic adb root because skip_root is set.')
+      return
+    if self.is_rootable:
+      self.root_adb()
 
   def load_snippet(self, name, package, config=None):
     """Starts the snippet apk with the given package name and connects.

tests/lib/mock_android_device.py

@@ -108,7 +108,7 @@ def __init__(
 
   def shell(self, params, timeout=None):
     if params == 'id -u':
-      return b'root'
+      return b'0'
     elif params == 'bugreportz':
       if self.fail_br:
         return b'OMG I died!\n'

tests/mobly/controllers/android_device_lib/services/logcat_test.py

@@ -829,7 +829,7 @@ def test__enable_logpersist_with_logpersist(self, MockFastboot, MockAdbProxy):
         'ro.build.type': 'userdebug',
         'ro.debuggable': '1',
     }
-    ad = android_device.AndroidDevice(serial=mock_serial)
+    ad = android_device.AndroidDevice(serial=mock_serial, skip_root=True)
     logcat_service = logcat.Logcat(ad)
     with self.assertNoLogs(level=logging.WARNING):
       logcat_service._enable_logpersist()
@@ -871,7 +871,7 @@ def test__enable_logpersist_with_user_build_device(
         'ro.build.type': 'user',
         'ro.debuggable': '0',
     }
-    ad = android_device.AndroidDevice(serial=mock_serial)
+    ad = android_device.AndroidDevice(serial=mock_serial, skip_root=True)
     logcat_service = logcat.Logcat(ad)
     logcat_service._enable_logpersist()
     mock_adb_proxy.shell.assert_not_called()
@@ -898,7 +898,7 @@ def test__enable_logpersist_with_missing_logpersist(
         'ro.debuggable': '1',
     }
     mock_adb_proxy.shell.side_effect = MOCK_LOGPERSIST_START_MISSING_ADB_ERROR
-    ad = android_device.AndroidDevice(serial=mock_serial)
+    ad = android_device.AndroidDevice(serial=mock_serial, skip_root=True)
     logcat_service = logcat.Logcat(ad)
     with self.assertLogs(level=logging.WARNING) as cm:
       logcat_service._enable_logpersist()
@@ -927,7 +927,7 @@ def test__enable_logpersist_with_failing_logpersist_stop(
         'ro.debuggable': '1',
     }
     mock_adb_proxy.shell.side_effect = MOCK_LOGPERSIST_STOP_MISSING_ADB_ERROR
-    ad = android_device.AndroidDevice(serial=mock_serial)
+    ad = android_device.AndroidDevice(serial=mock_serial, skip_root=True)
     logcat_service = logcat.Logcat(ad)
     with self.assertLogs(level=logging.WARNING) as cm:
       logcat_service._enable_logpersist()

tests/mobly/controllers/android_device_test.py

@@ -240,8 +240,26 @@ def test_get_instances_with_configs(
     mock_list_adb_usb.return_value = []
     configs = [{'serial': '1'}, {'serial': '2'}]
     android_device.get_instances_with_configs(configs)
-    mock_ad_class.assert_any_call('1')
-    mock_ad_class.assert_any_call('2')
+    mock_ad_class.assert_any_call('1', skip_root=False)
+    mock_ad_class.assert_any_call('2', skip_root=False)
+
+  @mock.patch('mobly.controllers.android_device.list_fastboot_devices')
+  @mock.patch('mobly.controllers.android_device.list_adb_devices')
+  @mock.patch('mobly.controllers.android_device.list_adb_devices_by_usb_id')
+  @mock.patch('mobly.controllers.android_device.AndroidDevice')
+  def test_get_instances_with_configs_skip_root(
+      self, mock_ad_class, mock_list_adb_usb, mock_list_adb, mock_list_fastboot
+  ):
+    mock_list_fastboot.return_value = []
+    mock_list_adb.return_value = ['1']
+    mock_list_adb_usb.return_value = []
+    configs = [{'serial': '1', 'skip_root': True, 'foo': 'bar'}]
+    android_device.get_instances_with_configs(configs)
+    mock_ad_class.assert_called_once_with('1', skip_root=True)
+    # `skip_root` is consumed by the constructor, not `load_config`.
+    mock_ad_class.return_value.load_config.assert_called_once_with(
+        {'foo': 'bar'}
+    )
 
   def test_get_instances_with_configs_invalid_config(self):
     config = {'something': 'random'}
@@ -263,7 +281,7 @@ def test_get_instances_with_configs_skips_optional_device_on_init_error(
     mock_list_adb_usb.return_value = []
     mock_ad = mock.MagicMock()
 
-    def _make_ad(serial):
+    def _make_ad(serial, skip_root=False):
       if serial == '1':
         raise Exception('Something went wrong.')
       return mock_ad
@@ -293,7 +311,7 @@ def test_get_instances_with_configs_skips_later_optional_device_on_init_error(
     mock_list_adb_usb.return_value = []
     mock_ad = mock.MagicMock()
 
-    def _make_ad(serial):
+    def _make_ad(serial, skip_root=False):
       if serial == '2':
         raise Exception('Something went wrong.')
       return mock_ad
@@ -749,6 +767,87 @@ def test_AndroidDevice_is_rootable_when_user_device(
     ad = android_device.AndroidDevice(serial='1')
     self.assertFalse(ad.is_rootable)
 
+  @mock.patch(
+      'mobly.controllers.android_device_lib.adb.AdbProxy',
+      return_value=mock_android_device.MockAdbProxy('1'),
+  )
+  @mock.patch(
+      'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
+      return_value=mock_android_device.MockFastbootProxy('1'),
+  )
+  def test_AndroidDevice_init_roots_rootable_device(
+      self, MockFastboot, MockAdbProxy
+  ):
+    MockAdbProxy.return_value.root = mock.MagicMock(return_value=b'')
+    ad = android_device.AndroidDevice(serial='1')
+    self.assertFalse(ad.skip_root)
+    ad.adb.root.assert_called_once_with()
+
+  @mock.patch(
+      'mobly.controllers.android_device_lib.adb.AdbProxy',
+      return_value=mock_android_device.MockAdbProxy(
+          '1',
+          mock_properties={
+              'ro.build.id': 'AB42',
+              'ro.build.type': 'user',
+              'ro.debuggable': '0',
+          },
+      ),
+  )
+  @mock.patch(
+      'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
+      return_value=mock_android_device.MockFastbootProxy('1'),
+  )
+  def test_AndroidDevice_init_does_not_root_user_device(
+      self, MockFastboot, MockAdbProxy
+  ):
+    MockAdbProxy.return_value.root = mock.MagicMock()
+    ad = android_device.AndroidDevice(serial='1')
+    ad.adb.root.assert_not_called()
+
+  @mock.patch(
+      'mobly.controllers.android_device_lib.adb.AdbProxy',
+      return_value=mock_android_device.MockAdbProxy('1'),
+  )
+  @mock.patch(
+      'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
+      return_value=mock_android_device.MockFastbootProxy('1'),
+  )
+  def test_AndroidDevice_init_honors_skip_root(
+      self, MockFastboot, MockAdbProxy
+  ):
+    MockAdbProxy.return_value.root = mock.MagicMock()
+    ad = android_device.AndroidDevice(serial='1', skip_root=True)
+    self.assertTrue(ad.skip_root)
+    ad.adb.root.assert_not_called()
+    # Explicit rooting still works when requested by the user.
+    self.assertTrue(ad.root_adb())
+    ad.adb.root.assert_called_once_with()
+
+  @mock.patch(
+      'mobly.controllers.android_device_lib.adb.AdbProxy',
+      return_value=mock_android_device.MockAdbProxy('1'),
+  )
+  @mock.patch(
+      'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
+      return_value=mock_android_device.MockFastbootProxy('1'),
+  )
+  def test_AndroidDevice_root_adb_warns_when_adbd_refuses_root(
+      self, MockFastboot, MockAdbProxy
+  ):
+    ad = android_device.AndroidDevice(serial='1', skip_root=True)
+    ad.adb.root = mock.MagicMock(
+        return_value=b'adbd cannot run as root in production builds'
+    )
+    ad.adb.shell = mock.MagicMock(return_value=b'2000')
+    with mock.patch.object(ad, 'log') as mock_log:
+      self.assertFalse(ad.root_adb())
+    ad.adb.root.assert_called_once_with()
+    mock_log.warning.assert_called_once()
+    self.assertIn(
+        'adbd cannot run as root', mock_log.warning.call_args[0][-1].decode()
+    )
+
   @mock.patch(
       'mobly.controllers.android_device_lib.adb.AdbProxy',
       return_value=mock_android_device.MockAdbProxy('1'),
@@ -2036,6 +2135,28 @@ def resume(self):
     self.assertTrue(ad.services.is_any_alive)
     self.assertFalse(ad.services.mock_service.resume_called)
 
+  @mock.patch(
+      'mobly.controllers.android_device_lib.adb.AdbProxy',
+      return_value=mock_android_device.MockAdbProxy('1'),
+  )
+  @mock.patch(
+      'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
+      return_value=mock_android_device.MockFastbootProxy('1'),
+  )
+  def test_AndroidDevice_handle_reboot_roots_unless_skip_root(
+      self, FastbootProxy, MockAdbProxy
+  ):
+    ad = android_device.AndroidDevice(serial='1')
+    ad.adb.root = mock.MagicMock(return_value=b'')
+    with ad.handle_reboot():
+      pass
+    ad.adb.root.assert_called_once_with()
+    ad.adb.root.reset_mock()
+    ad.skip_root = True
+    with ad.handle_reboot():
+      pass
+    ad.adb.root.assert_not_called()
+
   @mock.patch(
       'mobly.controllers.android_device_lib.adb.AdbProxy',
       return_value=mock_android_device.MockAdbProxy('1'),