From 5e1e70999257000f61183d0044a53f09059b2ab2 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Thu, 7 Feb 2019 10:32:27 -0800 Subject: [PATCH 1/2] Disable ContentCapture at runtime when it's disabled by DeviceConfig. Prior to this change, it would only be disabled on boot. Test: atest ChildlessActivityTest#testSetContentCaptureFeatureEnabled_disabledByDeviceConfig \ ChildlessActivityTest#testSetContentCaptureFeatureEnabled_disabledThenReEnabledByDeviceConfig Test: atest CtsContentCaptureServiceTestCases Bug: 123931489 Change-Id: I9230574db3ee85ba95951a3d9329c96f4e84a87e --- .../ContentCaptureManagerService.java | 67 +++++++++++++++++-- 1 file changed, 62 insertions(+), 5 deletions(-) diff --git a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java index 9249c9cc023a8..843555391cc26 100644 --- a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java +++ b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java @@ -23,6 +23,7 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.UserIdInt; import android.app.ActivityManagerInternal; +import android.app.ActivityThread; import android.content.ComponentName; import android.content.ContentResolver; import android.content.Context; @@ -39,6 +40,7 @@ import android.os.ResultReceiver; import android.os.ShellCallback; import android.os.UserHandle; import android.os.UserManager; +import android.provider.DeviceConfig; import android.provider.Settings; import android.util.LocalLog; import android.util.Slog; @@ -89,19 +91,31 @@ public final class ContentCaptureManagerService extends @Nullable private SparseBooleanArray mDisabledUsers; + /** + * Global kill-switch based on value defined by + * {@link android.provider.DeviceConfig.ContentCapture#PROPERTY_CONTENTCAPTURE_ENABLED}. + */ + @GuardedBy("mLock") + @Nullable + private boolean mDisabledByDeviceConfig; public ContentCaptureManagerService(@NonNull Context context) { super(context, new FrameworkResourcesServiceNameResolver(context, com.android.internal.R.string.config_defaultContentCaptureService), UserManager.DISALLOW_CONTENT_CAPTURE); - // Sets which serviecs are disabled + DeviceConfig.addOnPropertyChangedListener(DeviceConfig.ContentCapture.NAMESPACE, + ActivityThread.currentApplication().getMainExecutor(), + (namespace, name, value) -> setDisabledByDeviceConfig(value)); + setDisabledByDeviceConfig(); + + // Sets which services are disabled final UserManager um = getContext().getSystemService(UserManager.class); final List users = um.getUsers(); for (int i = 0; i < users.size(); i++) { final int userId = users.get(i).id; - final boolean disabled = isDisabledBySettings(userId); + final boolean disabled = mDisabledByDeviceConfig || isDisabledBySettings(userId); if (disabled) { - Slog.i(mTag, "user " + userId + " disabled by settings"); + Slog.i(mTag, "user " + userId + " disabled by settings or device config"); if (mDisabledUsers == null) { mDisabledUsers = new SparseBooleanArray(1); } @@ -160,7 +174,8 @@ public final class ContentCaptureManagerService extends @Override // from AbstractMasterSystemService protected boolean isDisabledLocked(@UserIdInt int userId) { - return isDisabledBySettingsLocked(userId) || super.isDisabledLocked(userId); + return mDisabledByDeviceConfig || isDisabledBySettingsLocked(userId) + || super.isDisabledLocked(userId); } private boolean isDisabledBySettingsLocked(@UserIdInt int userId) { @@ -191,6 +206,46 @@ public final class ContentCaptureManagerService extends return false; } + private void setDisabledByDeviceConfig() { + final String value = DeviceConfig.getProperty(DeviceConfig.ContentCapture.NAMESPACE, + DeviceConfig.ContentCapture.PROPERTY_CONTENTCAPTURE_ENABLED); + setDisabledByDeviceConfig(value); + } + + private void setDisabledByDeviceConfig(@Nullable String value) { + if (verbose) Slog.v(mTag, "setDisabledByDeviceConfig(): value=" + value); + final UserManager um = getContext().getSystemService(UserManager.class); + final List users = um.getUsers(); + + final boolean newDisabledValue; + + if (value != null && (value.equals("default") || value.equals("always"))) { + newDisabledValue = false; + if (debug) Slog.d(mTag, "setDisabledByDeviceConfig(): set to false on '" + value + "'"); + } else { + newDisabledValue = true; + } + + synchronized (mLock) { + if (mDisabledByDeviceConfig == newDisabledValue) { + if (verbose) { + Slog.v(mTag, "setDisabledByDeviceConfig(): already " + newDisabledValue); + } + return; + } + mDisabledByDeviceConfig = newDisabledValue; + + Slog.i(mTag, "setDisabledByDeviceConfig(): set to " + mDisabledByDeviceConfig); + for (int i = 0; i < users.size(); i++) { + final int userId = users.get(i).id; + boolean disabled = mDisabledByDeviceConfig || isDisabledBySettingsLocked(userId); + Slog.i(mTag, "setDisabledByDeviceConfig(): updating service for user " + + userId + " to " + (disabled ? "'disabled'" : "'enabled'")); + updateCachedServiceLocked(userId, disabled); + } + } + } + private void setContentCaptureFeatureEnabledForUser(@UserIdInt int userId, boolean enabled) { synchronized (mLock) { if (mDisabledUsers == null) { @@ -338,6 +393,8 @@ public final class ContentCaptureManagerService extends super.dumpLocked(prefix, pw); pw.print(prefix); pw.print("Disabled users: "); pw.println(mDisabledUsers); + pw.print(prefix); pw.print("Disabled by DeviceConfig: "); + pw.println(mDisabledByDeviceConfig); } final class ContentCaptureManagerServiceStub extends IContentCaptureManager.Stub { @@ -406,7 +463,7 @@ public final class ContentCaptureManagerService extends "isContentCaptureFeatureEnabled()", userId, Binder.getCallingUid(), result); if (!isService) return; - enabled = !isDisabledBySettingsLocked(userId); + enabled = !mDisabledByDeviceConfig && !isDisabledBySettingsLocked(userId); } try { result.send(enabled ? ContentCaptureManager.RESULT_CODE_TRUE From 14ef461fc2d59ac19116674e5d03f6316d567ba4 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Thu, 7 Feb 2019 12:24:38 -0800 Subject: [PATCH 2/2] Reverted ContentCapture kill-switch mechanism. Fixes: 121153631 Fixes: 121144410 Test: atest CtsContentCaptureServiceTestCases Change-Id: Ic0e3fe6e93a8c2aff12eebcf9872f0082a361589 --- api/system-current.txt | 5 +- api/test-current.txt | 7 +-- core/java/android/provider/DeviceConfig.java | 31 ++----------- .../contentcapture/ContentCaptureManager.java | 19 ++++++++ .../ContentCaptureManagerService.java | 24 ++++++---- .../java/com/android/server/SystemServer.java | 46 +++++++++---------- 6 files changed, 62 insertions(+), 70 deletions(-) diff --git a/api/system-current.txt b/api/system-current.txt index d1a8a56658eb7..7f1b57ee81cab 100644 --- a/api/system-current.txt +++ b/api/system-current.txt @@ -5709,6 +5709,7 @@ package android.provider { method @RequiresPermission(android.Manifest.permission.WRITE_DEVICE_CONFIG) public static void resetToDefaults(int, @Nullable String); method @RequiresPermission(android.Manifest.permission.WRITE_DEVICE_CONFIG) public static boolean setProperty(String, String, String, boolean); field public static final String NAMESPACE_AUTOFILL = "autofill"; + field public static final String NAMESPACE_CONTENT_CAPTURE = "content_capture"; field public static final String NAMESPACE_GAME_DRIVER = "game_driver"; field public static final String NAMESPACE_INPUT_NATIVE_BOOT = "input_native_boot"; field public static final String NAMESPACE_NETD_NATIVE = "netd_native"; @@ -5738,10 +5739,6 @@ package android.provider { field public static final String SERVICE_ENABLED = "service_enabled"; } - public static interface DeviceConfig.ContentCapture { - field public static final String NAMESPACE = "content_capture"; - } - public static interface DeviceConfig.DexBoot { field public static final String NAMESPACE = "dex_boot"; field public static final String PRIV_APPS_OOB_ENABLED = "priv_apps_oob_enabled"; diff --git a/api/test-current.txt b/api/test-current.txt index b977f8755fbb1..45ce3f05058ec 100644 --- a/api/test-current.txt +++ b/api/test-current.txt @@ -1787,11 +1787,7 @@ package android.provider { method @RequiresPermission("android.permission.READ_DEVICE_CONFIG") public static String getProperty(String, String); method @RequiresPermission("android.permission.WRITE_DEVICE_CONFIG") public static void resetToDefaults(int, @Nullable String); method @RequiresPermission("android.permission.WRITE_DEVICE_CONFIG") public static boolean setProperty(String, String, String, boolean); - } - - public static interface DeviceConfig.ContentCapture { - field public static final String NAMESPACE = "content_capture"; - field public static final String PROPERTY_CONTENTCAPTURE_ENABLED = "enable_contentcapture"; + field public static final String NAMESPACE_CONTENT_CAPTURE = "content_capture"; } public static interface DeviceConfig.Privacy { @@ -2723,6 +2719,7 @@ package android.view.contentcapture { public final class ContentCaptureManager { method public boolean isContentCaptureFeatureEnabled(); method public void setContentCaptureFeatureEnabled(boolean); + field public static final String DEVICE_CONFIG_PROPERTY_SERVICE_EXPLICITLY_ENABLED = "service_explicitly_enabled"; } public final class ViewNode extends android.app.assist.AssistStructure.ViewNode { diff --git a/core/java/android/provider/DeviceConfig.java b/core/java/android/provider/DeviceConfig.java index d67d98c3ab0ed..41d3cbb88f5c9 100644 --- a/core/java/android/provider/DeviceConfig.java +++ b/core/java/android/provider/DeviceConfig.java @@ -72,39 +72,14 @@ public final class DeviceConfig { public static final String NAMESPACE_AUTOFILL = "autofill"; /** - * ContentCapture-related properties definitions. + * Namespace for content capture feature used by on-device machine intelligence + * to provide suggestions in a privacy-safe manner. * * @hide */ @SystemApi @TestApi - public interface ContentCapture { - String NAMESPACE = "content_capture"; - - /** - * Property used by {@code com.android.server.SystemServer} on start to decide whether - * the Content Capture service should be created or not. - * - *

Possible values are: - * - *

    - *
  • If set to {@code default}, it will only be set if the OEM provides and defines the - * service name by overlaying {@code config_defaultContentCaptureService} (this is the - * "default" mode) - *
  • If set to {@code always}, it will always be enabled, even when the resource is not - * overlaid (this is useful during development and to run the CTS tests on AOSP builds). - *
  • Otherwise, it's explicitly disabled (this could work as a "kill switch" so OEMs - * can disable it remotely in case of emergency by setting to something else (like - * {@code "false"}); notice that it's also disabled if the OEM doesn't explicitly set one - * of the values above). - *
- * - * @hide - */ - // TODO(b/121153631): revert back to SERVICE_EXPLICITLY_ENABLED approach - @TestApi - String PROPERTY_CONTENTCAPTURE_ENABLED = "enable_contentcapture"; - } + public static final String NAMESPACE_CONTENT_CAPTURE = "content_capture"; /** * Namespace for all input-related features that are used at the native level. diff --git a/core/java/android/view/contentcapture/ContentCaptureManager.java b/core/java/android/view/contentcapture/ContentCaptureManager.java index 2512b95fe0ec6..634443d78b490 100644 --- a/core/java/android/view/contentcapture/ContentCaptureManager.java +++ b/core/java/android/view/contentcapture/ContentCaptureManager.java @@ -66,6 +66,25 @@ public final class ContentCaptureManager { */ private static final int SYNC_CALLS_TIMEOUT_MS = 5000; + /** + * DeviceConfig property used by {@code com.android.server.SystemServer} on start to decide + * whether the Content Capture service should be created or not + * + *

By default it should *NOT* be set (or set to {@code "default"}, so the decision is based + * on whether the OEM provides an implementation for the service), but it can be overridden to: + * + *

    + *
  • Provide a "kill switch" so OEMs can disable it remotely in case of emergency (when + * it's set to {@code "false"}). + *
  • Enable the CTS tests to be run on AOSP builds (when it's set to {@code "true"}). + *
+ * + * @hide + */ + @TestApi + public static final String DEVICE_CONFIG_PROPERTY_SERVICE_EXPLICITLY_ENABLED = + "service_explicitly_enabled"; + private final Object mLock = new Object(); @NonNull diff --git a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java index 843555391cc26..4afbc641ea6c0 100644 --- a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java +++ b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java @@ -93,7 +93,7 @@ public final class ContentCaptureManagerService extends /** * Global kill-switch based on value defined by - * {@link android.provider.DeviceConfig.ContentCapture#PROPERTY_CONTENTCAPTURE_ENABLED}. + * {@link ContentCaptureManager#DEVICE_CONFIG_PROPERTY_SERVICE_EXPLICITLY_ENABLED}. */ @GuardedBy("mLock") @Nullable @@ -103,9 +103,16 @@ public final class ContentCaptureManagerService extends super(context, new FrameworkResourcesServiceNameResolver(context, com.android.internal.R.string.config_defaultContentCaptureService), UserManager.DISALLOW_CONTENT_CAPTURE); - DeviceConfig.addOnPropertyChangedListener(DeviceConfig.ContentCapture.NAMESPACE, + DeviceConfig.addOnPropertyChangedListener(DeviceConfig.NAMESPACE_CONTENT_CAPTURE, ActivityThread.currentApplication().getMainExecutor(), - (namespace, name, value) -> setDisabledByDeviceConfig(value)); + (namespace, key, value) -> { + if (!ContentCaptureManager.DEVICE_CONFIG_PROPERTY_SERVICE_EXPLICITLY_ENABLED + .equals(key)) { + Slog.i(mTag, "Ignoring change on " + key); + return; + } + setDisabledByDeviceConfig(value); + }); setDisabledByDeviceConfig(); // Sets which services are disabled @@ -207,8 +214,8 @@ public final class ContentCaptureManagerService extends } private void setDisabledByDeviceConfig() { - final String value = DeviceConfig.getProperty(DeviceConfig.ContentCapture.NAMESPACE, - DeviceConfig.ContentCapture.PROPERTY_CONTENTCAPTURE_ENABLED); + final String value = DeviceConfig.getProperty(DeviceConfig.NAMESPACE_CONTENT_CAPTURE, + ContentCaptureManager.DEVICE_CONFIG_PROPERTY_SERVICE_EXPLICITLY_ENABLED); setDisabledByDeviceConfig(value); } @@ -219,11 +226,10 @@ public final class ContentCaptureManagerService extends final boolean newDisabledValue; - if (value != null && (value.equals("default") || value.equals("always"))) { - newDisabledValue = false; - if (debug) Slog.d(mTag, "setDisabledByDeviceConfig(): set to false on '" + value + "'"); - } else { + if (value != null && value.equalsIgnoreCase("false")) { newDisabledValue = true; + } else { + newDisabledValue = false; } synchronized (mLock) { diff --git a/services/java/com/android/server/SystemServer.java b/services/java/com/android/server/SystemServer.java index ab30cda271f0b..a6017f2c1e86d 100644 --- a/services/java/com/android/server/SystemServer.java +++ b/services/java/com/android/server/SystemServer.java @@ -64,6 +64,7 @@ import android.util.EventLog; import android.util.Slog; import android.util.TimingsTraceLog; import android.view.WindowManager; +import android.view.contentcapture.ContentCaptureManager; import android.view.inputmethod.InputMethodSystemProperty; import com.android.internal.R; @@ -2214,33 +2215,30 @@ public final class SystemServer { } private void startContentCaptureService(@NonNull Context context) { - // Check if it was explicitly enabled by DeviceConfig - final String settings = DeviceConfig.getProperty(DeviceConfig.ContentCapture.NAMESPACE, - DeviceConfig.ContentCapture.PROPERTY_CONTENTCAPTURE_ENABLED); - if (settings == null) { - // Better be safe than sorry... - Slog.d(TAG, "ContentCaptureService disabled because its not set by OEM"); - return; - } - switch (settings) { - case "always": - // Should be used only during development + // First check if it was explicitly enabled by DeviceConfig + boolean explicitlySupported = false; + String settings = DeviceConfig.getProperty(DeviceConfig.NAMESPACE_CONTENT_CAPTURE, + ContentCaptureManager.DEVICE_CONFIG_PROPERTY_SERVICE_EXPLICITLY_ENABLED); + if (settings != null && !settings.equalsIgnoreCase("default")) { + explicitlySupported = Boolean.parseBoolean(settings); + if (explicitlySupported) { Slog.d(TAG, "ContentCaptureService explicitly enabled by DeviceConfig"); - break; - case "default": - // Default case: check if OEM overlaid the resource that defines the service. - final String serviceName = context.getString( - com.android.internal.R.string.config_defaultContentCaptureService); - if (TextUtils.isEmpty(serviceName)) { - Slog.d(TAG, "ContentCaptureService disabled because resource is not overlaid"); - return; - } - break; - default: - // Kill switch for OEMs - Slog.d(TAG, "ContentCaptureService disabled because its set to: " + settings); + } else { + Slog.d(TAG, "ContentCaptureService explicitly disabled by DeviceConfig"); return; + } } + + // Then check if OEM overlaid the resource that defines the service. + if (!explicitlySupported) { + final String serviceName = context + .getString(com.android.internal.R.string.config_defaultContentCaptureService); + if (TextUtils.isEmpty(serviceName)) { + Slog.d(TAG, "ContentCaptureService disabled because resource is not overlaid"); + return; + } + } + traceBeginAndSlog("StartContentCaptureService"); mSystemServiceManager.startService(CONTENT_CAPTURE_MANAGER_SERVICE_CLASS); traceEnd();