From 43dc489afd8274bc9d2a1a0ac9e12caaef34da09 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Fri, 11 Jun 2021 13:57:38 -0700 Subject: [PATCH] Split DevicePolicyManagerInternal into DevicePolicyManagerLiteInternal. DevicePolicyManagerInternal is not set when the device doesn't have the device_admin feature, but some methods are still needed in that scenario (like notifyUnsafeOperationStateChanged() on automotive). Fixes: 190395562 Test: manual verification Test: atest FrameworksServicesTests:DevicePolicyManagerTest FrameworksServicesTests:DevicePolicyManagerServiceMigrationTest Change-Id: I48271b828ff01c4e4f3e0365410a7a745f1b0a1d --- .../admin/DevicePolicyManagerInternal.java | 10 ----- .../DevicePolicyManagerLiteInternal.java | 41 +++++++++++++++++++ .../DevicePolicyManagerService.java | 6 ++- .../devicepolicy/OneTimeSafetyChecker.java | 6 +-- ...vicePolicyManagerServiceMigrationTest.java | 5 +++ .../devicepolicy/DevicePolicyManagerTest.java | 6 +++ 6 files changed, 60 insertions(+), 14 deletions(-) create mode 100644 core/java/android/app/admin/DevicePolicyManagerLiteInternal.java diff --git a/core/java/android/app/admin/DevicePolicyManagerInternal.java b/core/java/android/app/admin/DevicePolicyManagerInternal.java index a9bec98ce405b..a0d2977cf09a9 100644 --- a/core/java/android/app/admin/DevicePolicyManagerInternal.java +++ b/core/java/android/app/admin/DevicePolicyManagerInternal.java @@ -18,7 +18,6 @@ package android.app.admin; import android.annotation.Nullable; import android.annotation.UserIdInt; -import android.app.admin.DevicePolicyManager.OperationSafetyReason; import android.content.ComponentName; import android.content.Intent; import android.os.UserHandle; @@ -256,13 +255,4 @@ public abstract class DevicePolicyManagerInternal { * {@link #supportsResetOp(int)} is true. */ public abstract void resetOp(int op, String packageName, @UserIdInt int userId); - - /** - * Notifies the system that an unsafe operation reason has changed. - * - * @throws IllegalArgumentException if {@code checker} is not the same as set on - * {@code DevicePolicyManagerService}. - */ - public abstract void notifyUnsafeOperationStateChanged(DevicePolicySafetyChecker checker, - @OperationSafetyReason int reason, boolean isSafe); } diff --git a/core/java/android/app/admin/DevicePolicyManagerLiteInternal.java b/core/java/android/app/admin/DevicePolicyManagerLiteInternal.java new file mode 100644 index 0000000000000..ccb99470d372e --- /dev/null +++ b/core/java/android/app/admin/DevicePolicyManagerLiteInternal.java @@ -0,0 +1,41 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.app.admin; + +import android.app.admin.DevicePolicyManager.OperationSafetyReason; + +/** + * Device policy manager local system service interface for methods that don't require the + * {@code device_admin} feature. + * + * Maintenance note: if you need to expose information from DPMS to lower level services such as + * PM/UM/AM/etc, then exposing it from DevicePolicyManagerInternal is not safe because it may cause + * lock order inversion. Consider using {@link DevicePolicyCache} instead. + * + * @hide Only for use within the system server. + */ +public interface DevicePolicyManagerLiteInternal { + + /** + * Notifies the system that an unsafe operation reason has changed. + * + * @throws IllegalArgumentException if {@code checker} is not the same as set on + * {@code DevicePolicyManagerService.setDevicePolicySafetyChecker()}. + */ + void notifyUnsafeOperationStateChanged(DevicePolicySafetyChecker checker, + @OperationSafetyReason int reason, boolean isSafe); +} diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index 0128d350bd101..5d53c9840e947 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -169,6 +169,7 @@ import android.app.admin.DevicePolicyManager.OperationSafetyReason; import android.app.admin.DevicePolicyManager.PasswordComplexity; import android.app.admin.DevicePolicyManager.PersonalAppsSuspensionReason; import android.app.admin.DevicePolicyManagerInternal; +import android.app.admin.DevicePolicyManagerLiteInternal; import android.app.admin.DevicePolicySafetyChecker; import android.app.admin.DeviceStateCache; import android.app.admin.FactoryResetProtectionPolicy; @@ -1748,6 +1749,8 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { mTransferOwnershipMetadataManager = mInjector.newTransferOwnershipMetadataManager(); mBugreportCollectionManager = new RemoteBugreportManager(this, mInjector); + // "Lite" interface is available even when the device doesn't have the feature + LocalServices.addService(DevicePolicyManagerLiteInternal.class, mLocalService); if (!mHasFeature) { // Skip the rest of the initialization mSetupContentObserver = null; @@ -12612,7 +12615,8 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } @VisibleForTesting - final class LocalService extends DevicePolicyManagerInternal { + final class LocalService extends DevicePolicyManagerInternal + implements DevicePolicyManagerLiteInternal { private List mWidgetProviderListeners; @Override diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/OneTimeSafetyChecker.java b/services/devicepolicy/java/com/android/server/devicepolicy/OneTimeSafetyChecker.java index 86437a27a64dc..1cbc634b71525 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/OneTimeSafetyChecker.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/OneTimeSafetyChecker.java @@ -21,7 +21,7 @@ import static android.app.admin.DevicePolicyManager.operationToString; import android.app.admin.DevicePolicyManager.DevicePolicyOperation; import android.app.admin.DevicePolicyManager.OperationSafetyReason; -import android.app.admin.DevicePolicyManagerInternal; +import android.app.admin.DevicePolicyManagerLiteInternal; import android.app.admin.DevicePolicySafetyChecker; import android.os.Handler; import android.os.Looper; @@ -80,8 +80,8 @@ final class OneTimeSafetyChecker implements DevicePolicySafetyChecker { + ", should be " + operationToString(mOperation)); } String reasonName = operationSafetyReasonToString(reason); - DevicePolicyManagerInternal dpmi = LocalServices - .getService(DevicePolicyManagerInternal.class); + DevicePolicyManagerLiteInternal dpmi = LocalServices + .getService(DevicePolicyManagerLiteInternal.class); Slog.i(TAG, "notifying " + reasonName + " is UNSAFE"); dpmi.notifyUnsafeOperationStateChanged(this, reason, /* isSafe= */ false); diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceMigrationTest.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceMigrationTest.java index fa3f45c08202b..a2b1c1cb1a490 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceMigrationTest.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceMigrationTest.java @@ -34,6 +34,7 @@ import static org.mockito.Mockito.when; import android.app.admin.DevicePolicyManager; import android.app.admin.DevicePolicyManagerInternal; +import android.app.admin.DevicePolicyManagerLiteInternal; import android.content.ComponentName; import android.content.Intent; import android.content.pm.PackageManager; @@ -158,6 +159,7 @@ public class DevicePolicyManagerServiceMigrationTest extends DpmTestBase { final long ident = mContext.binder.clearCallingIdentity(); try { + LocalServices.removeServiceForTest(DevicePolicyManagerLiteInternal.class); LocalServices.removeServiceForTest(DevicePolicyManagerInternal.class); dpms = new DevicePolicyManagerServiceTestable(getServices(), mContext); @@ -271,6 +273,7 @@ public class DevicePolicyManagerServiceMigrationTest extends DpmTestBase { final long ident = mContext.binder.clearCallingIdentity(); try { + LocalServices.removeServiceForTest(DevicePolicyManagerLiteInternal.class); LocalServices.removeServiceForTest(DevicePolicyManagerInternal.class); dpms = new DevicePolicyManagerServiceTestable(getServices(), mContext); @@ -339,6 +342,7 @@ public class DevicePolicyManagerServiceMigrationTest extends DpmTestBase { // (Need clearCallingIdentity() to pass permission checks.) final long ident = mContext.binder.clearCallingIdentity(); try { + LocalServices.removeServiceForTest(DevicePolicyManagerLiteInternal.class); LocalServices.removeServiceForTest(DevicePolicyManagerInternal.class); dpms = new DevicePolicyManagerServiceTestable(getServices(), mContext); @@ -499,6 +503,7 @@ public class DevicePolicyManagerServiceMigrationTest extends DpmTestBase { DevicePolicyManagerServiceTestable dpms; final long ident = mContext.binder.clearCallingIdentity(); try { + LocalServices.removeServiceForTest(DevicePolicyManagerLiteInternal.class); LocalServices.removeServiceForTest(DevicePolicyManagerInternal.class); dpms = new DevicePolicyManagerServiceTestable(getServices(), mContext); diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java index 5447a58a1643d..cedf6361e33b6 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java @@ -84,6 +84,7 @@ import android.app.PendingIntent; import android.app.admin.DeviceAdminReceiver; import android.app.admin.DevicePolicyManager; import android.app.admin.DevicePolicyManagerInternal; +import android.app.admin.DevicePolicyManagerLiteInternal; import android.app.admin.FactoryResetProtectionPolicy; import android.app.admin.PasswordMetrics; import android.app.admin.SystemUpdatePolicy; @@ -280,6 +281,7 @@ public class DevicePolicyManagerTest extends DpmTestBase { private void initializeDpms() { // Need clearCallingIdentity() to pass permission checks. final long ident = mContext.binder.clearCallingIdentity(); + LocalServices.removeServiceForTest(DevicePolicyManagerLiteInternal.class); LocalServices.removeServiceForTest(DevicePolicyManagerInternal.class); dpms = new DevicePolicyManagerServiceTestable(getServices(), mContext); @@ -369,10 +371,14 @@ public class DevicePolicyManagerTest extends DpmTestBase { .thenReturn(false); LocalServices.removeServiceForTest(DevicePolicyManagerInternal.class); + LocalServices.removeServiceForTest(DevicePolicyManagerLiteInternal.class); new DevicePolicyManagerServiceTestable(getServices(), mContext); // If the device has no DPMS feature, it shouldn't register the local service. assertThat(LocalServices.getService(DevicePolicyManagerInternal.class)).isNull(); + + // But should still register the lite one + assertThat(LocalServices.getService(DevicePolicyManagerLiteInternal.class)).isNotNull(); } @Test