Refactored CarUserService calls to removeUserOrSetEphemeral().
Test: atest com.android.car.user.CarUserServiceTest \
android.car.apitest.CarDevicePolicyManagerTest#testRemoveUser_whenDisallowed
Fixes: 170887769
Change-Id: If797ace64c0fa0262116f649212bbcb1d61e2046
diff --git a/car-test-lib/src/android/car/test/mocks/AndroidMockitoHelper.java b/car-test-lib/src/android/car/test/mocks/AndroidMockitoHelper.java
index efdd4e9..150ab73 100644
--- a/car-test-lib/src/android/car/test/mocks/AndroidMockitoHelper.java
+++ b/car-test-lib/src/android/car/test/mocks/AndroidMockitoHelper.java
@@ -24,13 +24,10 @@
import android.annotation.Nullable;
import android.annotation.UserIdInt;
import android.app.ActivityManager;
-import android.car.test.util.AndroidHelper;
import android.car.test.util.UserTestingHelper;
import android.car.test.util.UserTestingHelper.UserInfoBuilder;
import android.car.test.util.Visitor;
-import android.content.BroadcastReceiver;
import android.content.Context;
-import android.content.Intent;
import android.content.pm.PackageManager;
import android.content.pm.UserInfo;
import android.content.pm.UserInfo.UserInfoFlag;
@@ -40,6 +37,7 @@
import android.os.ServiceManager;
import android.os.UserHandle;
import android.os.UserManager;
+import android.os.UserManager.RemoveResult;
import android.util.Log;
import java.util.List;
@@ -178,44 +176,19 @@
}
/**
- * Mocks a successful call to {@code UserManager#removeUserOrSetEphemeral(int)}.
- */
- public static void mockUmRemoveUserOrSetEphemeral(@NonNull UserManager um,
- @NonNull UserInfo user) {
- mockUmRemoveUserOrSetEphemeral(um, user, /* listener= */ null);
- }
-
- /**
- * Mocks a successful call to {@code UserManager#removeUserOrSetEphemeral(int)}, including the
- * respective {@code receiver} notification.
- */
- public static void mockUmRemoveUserOrSetEphemeral(@NonNull Context context,
- @NonNull UserManager um, @NonNull BroadcastReceiver receiver, @NonNull UserInfo user) {
- mockUmRemoveUserOrSetEphemeral(um, user, (u) -> {
- int userId = u.id;
- Intent intent = new Intent(Intent.ACTION_USER_REMOVED);
- intent.putExtra(Intent.EXTRA_USER_ID, userId);
- intent.putExtra(Intent.EXTRA_USER_HANDLE, userId);
- intent.putExtra(Intent.EXTRA_USER, UserHandle.of(userId));
- Log.v(TAG, "mockUmRemoveUserOrSetEphemeral(): broadcasting "
- + AndroidHelper.toString(intent) + " to " + receiver);
- receiver.onReceive(context, intent);
- });
- }
-
- /**
* Mocks a successful call to {@code UserManager#removeUserOrSetEphemeral(int)}, and notifies
* {@code listener} when it's called.
*/
public static void mockUmRemoveUserOrSetEphemeral(@NonNull UserManager um,
- @NonNull UserInfo user, @Nullable Visitor<UserInfo> listener) {
+ @NonNull UserInfo user, boolean evenWhenDisallowed, @RemoveResult int result,
+ @Nullable Visitor<UserInfo> listener) {
int userId = user.id;
- when(um.removeUserOrSetEphemeral(userId)).thenAnswer((inv) -> {
+ when(um.removeUserOrSetEphemeral(userId, evenWhenDisallowed)).thenAnswer((inv) -> {
if (listener != null) {
Log.v(TAG, "mockUmRemoveUserOrSetEphemeral(" + user + "): notifying " + listener);
listener.visit(user);
}
- return UserManager.REMOVE_RESULT_REMOVED;
+ return result;
});
}
diff --git a/service/src/com/android/car/user/CarUserService.java b/service/src/com/android/car/user/CarUserService.java
index 5f3daf5..e05be5e 100644
--- a/service/src/com/android/car/user/CarUserService.java
+++ b/service/src/com/android/car/user/CarUserService.java
@@ -1144,7 +1144,12 @@
// First remove user from android and then remove from HAL because HAL remove user is one
// way call.
- int result = mUserManager.removeUserOrSetEphemeral(userId);
+ // TODO(b/170887769): rename hasCallerRestrictions to fromCarDevicePolicyManager (or use an
+ // int / enum to indicate if it's called from CarUserManager or CarDevicePolicyManager), as
+ // it's counter-intuitive that it's "allowed even when disallowed" when it
+ // "has caller restrictions"
+ boolean evenWhenDisallowed = hasCallerRestrictions;
+ int result = mUserManager.removeUserOrSetEphemeral(userId, evenWhenDisallowed);
if (result == UserManager.REMOVE_RESULT_ERROR) {
return logAndGetResults(userId, UserRemovalResult.STATUS_ANDROID_FAILURE);
}
diff --git a/tests/android_car_api_test/src/android/car/apitest/CarApiTestBase.java b/tests/android_car_api_test/src/android/car/apitest/CarApiTestBase.java
index f16c689..624443c 100644
--- a/tests/android_car_api_test/src/android/car/apitest/CarApiTestBase.java
+++ b/tests/android_car_api_test/src/android/car/apitest/CarApiTestBase.java
@@ -84,7 +84,7 @@
private Car mCar;
private CarUserManager mCarUserManager;
- private UserManager mUserManager;
+ protected UserManager mUserManager;
protected final DefaultServiceConnectionListener mConnectionListener =
new DefaultServiceConnectionListener();
@@ -92,7 +92,7 @@
private final List<Integer> mUsersToRemove = new ArrayList<>();
@Before
- public final void connectToCar() throws Exception {
+ public final void setFixturesAndConnectToCar() throws Exception {
mCar = Car.createCar(getContext(), mConnectionListener);
mCar.connect();
mConnectionListener.waitForConnection(DEFAULT_WAIT_TIMEOUT_MS);
diff --git a/tests/android_car_api_test/src/android/car/apitest/CarDevicePolicyManagerTest.java b/tests/android_car_api_test/src/android/car/apitest/CarDevicePolicyManagerTest.java
index de74561..e75be34 100644
--- a/tests/android_car_api_test/src/android/car/apitest/CarDevicePolicyManagerTest.java
+++ b/tests/android_car_api_test/src/android/car/apitest/CarDevicePolicyManagerTest.java
@@ -32,6 +32,7 @@
import android.content.pm.UserInfo;
import android.os.PowerManager;
import android.os.UserHandle;
+import android.os.UserManager;
import android.util.Log;
import org.junit.Before;
@@ -68,6 +69,16 @@
}
@Test
+ public void testRemoveUser_whenDisallowed() throws Exception {
+ try {
+ testRemoveUser();
+ } finally {
+ mUserManager.setUserRestriction(UserManager.DISALLOW_REMOVE_USER, false,
+ UserHandle.SYSTEM);
+ }
+ }
+
+ @Test
public void testRemoveUser_currentUserSetEphemeral() throws Exception {
int startUser = ActivityManager.getCurrentUser();
UserInfo user = createUser(
diff --git a/tests/carservice_unit_test/src/com/android/car/user/CarUserServiceTest.java b/tests/carservice_unit_test/src/com/android/car/user/CarUserServiceTest.java
index e358129..7d84b30 100644
--- a/tests/carservice_unit_test/src/com/android/car/user/CarUserServiceTest.java
+++ b/tests/carservice_unit_test/src/com/android/car/user/CarUserServiceTest.java
@@ -36,6 +36,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyInt;
@@ -103,6 +104,7 @@
import android.os.RemoteException;
import android.os.UserHandle;
import android.os.UserManager;
+import android.os.UserManager.RemoveResult;
import android.sysprop.CarProperties;
import android.util.Log;
import android.util.SparseArray;
@@ -785,12 +787,11 @@
UserInfo currentUser = mRegularUser;
mockExistingUsersAndCurrentUser(mExistingUsers, currentUser);
UserInfo removeUser = mRegularUser;
- when(mMockedUserManager.removeUserOrSetEphemeral(removeUser.id)).thenReturn(
- UserManager.REMOVE_RESULT_SET_EPHEMERAL);
+ mockRemoveUserNoCallback(removeUser, UserManager.REMOVE_RESULT_SET_EPHEMERAL);
UserRemovalResult result = mCarUserService.removeUser(removeUser.id);
- assertThat(result.getStatus()).isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL_SET_EPHEMERAL);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_SUCCESSFUL_SET_EPHEMERAL);
assertNoHalUserRemoval();
}
@@ -799,13 +800,11 @@
UserInfo currentUser = mRegularUser;
mockExistingUsersAndCurrentUser(mExistingUsers, currentUser);
UserInfo removeUser = mRegularUser;
- when(mMockedUserManager.removeUserOrSetEphemeral(removeUser.id)).thenReturn(
- UserManager.REMOVE_RESULT_ALREADY_BEING_REMOVED);
- mockRemoveUser(removeUser);
+ mockRemoveUser(removeUser, UserManager.REMOVE_RESULT_ALREADY_BEING_REMOVED);
UserRemovalResult result = mCarUserService.removeUser(removeUser.id);
- assertThat(result.getStatus()).isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_SUCCESSFUL);
assertHalRemove(currentUser, removeUser);
}
@@ -815,14 +814,13 @@
List<UserInfo> existingUsers = Arrays.asList(mAdminUser, mRegularUser);
mockExistingUsersAndCurrentUser(existingUsers, currentUser);
UserInfo removeUser = mAdminUser;
- when(mMockedUserManager.removeUserOrSetEphemeral(removeUser.id)).thenReturn(
- UserManager.REMOVE_RESULT_SET_EPHEMERAL);
+ mockRemoveUserNoCallback(removeUser, UserManager.REMOVE_RESULT_SET_EPHEMERAL);
UserRemovalResult result = mCarUserService.removeUser(mAdminUser.id,
NO_CALLER_RESTRICTIONS);
- assertThat(result.getStatus())
- .isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL_LAST_ADMIN_SET_EPHEMERAL);
+ assertUserRemovalResultStatus(result,
+ UserRemovalResult.STATUS_SUCCESSFUL_LAST_ADMIN_SET_EPHEMERAL);
assertNoHalUserRemoval();
}
@@ -831,8 +829,7 @@
UserRemovalResult result = mCarUserService.removeUser(15,
NO_CALLER_RESTRICTIONS);
- assertThat(result.getStatus())
- .isEqualTo(UserRemovalResult.STATUS_USER_DOES_NOT_EXIST);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_USER_DOES_NOT_EXIST);
}
@Test
@@ -847,8 +844,8 @@
UserRemovalResult result = mCarUserService.removeUser(mAdminUser.id,
NO_CALLER_RESTRICTIONS);
- assertThat(result.getStatus())
- .isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL_LAST_ADMIN_REMOVED);
+ assertUserRemovalResultStatus(result,
+ UserRemovalResult.STATUS_SUCCESSFUL_LAST_ADMIN_REMOVED);
assertHalRemove(currentUser, removeUser);
}
@@ -864,7 +861,7 @@
UserRemovalResult result = mCarUserService.removeUser(removeUser.id,
NO_CALLER_RESTRICTIONS);
- assertThat(result.getStatus()).isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_SUCCESSFUL);
assertHalRemove(currentUser, removeUser);
}
@@ -878,7 +875,7 @@
UserRemovalResult result = mCarUserService.removeUser(removeUser.id,
NO_CALLER_RESTRICTIONS);
- assertThat(result.getStatus()).isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_SUCCESSFUL);
assertHalRemove(currentUser, removeUser);
}
@@ -892,7 +889,7 @@
UserRemovalResult result = mCarUserService.removeUser(removeUser.id,
NO_CALLER_RESTRICTIONS);
- assertThat(result.getStatus()).isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_SUCCESSFUL);
verify(mUserHal, never()).removeUser(any());
}
@@ -900,13 +897,12 @@
public void testRemoveUser_androidFailure() throws Exception {
mockExistingUsersAndCurrentUser(mAdminUser);
int targetUserId = mRegularUser.id;
- when(mMockedUserManager.removeUserOrSetEphemeral(targetUserId)).thenReturn(
- UserManager.REMOVE_RESULT_ERROR);
+ mockRemoveUser(new UserInfoBuilder(targetUserId).build(), UserManager.REMOVE_RESULT_ERROR);
UserRemovalResult result = mCarUserService.removeUser(targetUserId,
NO_CALLER_RESTRICTIONS);
- assertThat(result.getStatus()).isEqualTo(UserRemovalResult.STATUS_ANDROID_FAILURE);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_ANDROID_FAILURE);
}
@Test
@@ -915,7 +911,7 @@
UserInfo removeUser = mAdminUser;
mockGetCallingUserHandle(currentUser.id);
mockExistingUsersAndCurrentUser(mExistingUsers, currentUser);
- mockRemoveUser(removeUser);
+ mockRemoveUser(removeUser, /* evenWhenDisallowed= */ true);
assertThrows(SecurityException.class, () -> mCarUserService.removeUser(removeUser.id,
HAS_CALLER_RESTRICTIONS));
@@ -927,7 +923,7 @@
UserInfo removeUser = mAnotherRegularUser;
mockGetCallingUserHandle(currentUser.id);
mockExistingUsersAndCurrentUser(mExistingUsers, currentUser);
- mockRemoveUser(removeUser);
+ mockRemoveUser(removeUser, /* evenWhenDisallowed= */ true);
assertThrows(SecurityException.class, () -> mCarUserService.removeUser(removeUser.id,
HAS_CALLER_RESTRICTIONS));
@@ -939,14 +935,13 @@
UserInfo removeUser = mRegularUser;
mockGetCallingUserHandle(currentUser.id);
mockExistingUsersAndCurrentUser(mExistingUsers, currentUser);
- when(mMockedUserManager.removeUserOrSetEphemeral(removeUser.id)).thenReturn(
+ mockRemoveUserNoCallback(removeUser, /* evenWhenDisallowed= */ true,
UserManager.REMOVE_RESULT_SET_EPHEMERAL);
UserRemovalResult result = mCarUserService.removeUser(removeUser.id,
HAS_CALLER_RESTRICTIONS);
- assertThat(result.getStatus())
- .isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL_SET_EPHEMERAL);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_SUCCESSFUL_SET_EPHEMERAL);
assertNoHalUserRemoval();
}
@@ -956,13 +951,13 @@
UserInfo removeUser = mAnotherAdminUser;
mockGetCallingUserHandle(currentUser.id);
mockExistingUsersAndCurrentUser(mExistingUsers, currentUser);
- mockRemoveUser(removeUser);
+ mockRemoveUser(removeUser, /* evenWhenDisallowed= */ true);
UserRemovalResult result = mCarUserService.removeUser(removeUser.id,
HAS_CALLER_RESTRICTIONS);
- assertThat(result.getStatus()).isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL);
- assertHalRemove(currentUser, removeUser);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_SUCCESSFUL);
+ assertHalRemove(currentUser, removeUser, /* evenWhenDisallowed= */ true);
}
@Test
@@ -971,13 +966,13 @@
UserInfo removeUser = mRegularUser;
mockGetCallingUserHandle(currentUser.id);
mockExistingUsersAndCurrentUser(mExistingUsers, currentUser);
- mockRemoveUser(removeUser);
+ mockRemoveUser(removeUser, /* evenWhenDisallowed= */ true);
UserRemovalResult result = mCarUserService.removeUser(removeUser.id,
HAS_CALLER_RESTRICTIONS);
- assertThat(result.getStatus()).isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL);
- assertHalRemove(currentUser, removeUser);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_SUCCESSFUL);
+ assertHalRemove(currentUser, removeUser, /* evenWhenDisallowed= */ true);
}
@Test
@@ -986,14 +981,13 @@
UserInfo removeUser = mAdminUser;
mockGetCallingUserHandle(currentUser.id);
mockExistingUsersAndCurrentUser(mExistingUsers, currentUser);
- when(mMockedUserManager.removeUserOrSetEphemeral(removeUser.id)).thenReturn(
+ mockRemoveUserNoCallback(removeUser, /* evenWhenDisallowed= */ true,
UserManager.REMOVE_RESULT_SET_EPHEMERAL);
UserRemovalResult result = mCarUserService.removeUser(removeUser.id,
HAS_CALLER_RESTRICTIONS);
- assertThat(result.getStatus())
- .isEqualTo(UserRemovalResult.STATUS_SUCCESSFUL_SET_EPHEMERAL);
+ assertUserRemovalResultStatus(result, UserRemovalResult.STATUS_SUCCESSFUL_SET_EPHEMERAL);
assertNoHalUserRemoval();
}
@@ -2211,10 +2205,33 @@
}
private void mockRemoveUser(@NonNull UserInfo user) {
- mockUmRemoveUserOrSetEphemeral(mMockedUserManager, user,
+ mockRemoveUser(user, UserManager.REMOVE_RESULT_REMOVED);
+ }
+
+ private void mockRemoveUser(@NonNull UserInfo user, @RemoveResult int result) {
+ mockRemoveUser(user, /* evenWhenDisallowed= */ false, result);
+ }
+
+ private void mockRemoveUser(@NonNull UserInfo user, boolean evenWhenDisallowed) {
+ mockRemoveUser(user, evenWhenDisallowed, UserManager.REMOVE_RESULT_REMOVED);
+ }
+
+ private void mockRemoveUser(@NonNull UserInfo user, boolean evenWhenDisallowed,
+ @RemoveResult int result) {
+ mockUmRemoveUserOrSetEphemeral(mMockedUserManager, user, evenWhenDisallowed, result,
(u) -> mCarUserService.onUserRemoved(u));
}
+ private void mockRemoveUserNoCallback(@NonNull UserInfo user, @RemoveResult int result) {
+ mockRemoveUserNoCallback(user, /* evenWhenDisallowed= */ false, result);
+ }
+
+ private void mockRemoveUserNoCallback(@NonNull UserInfo user, boolean evenWhenDisallowed,
+ @RemoveResult int result) {
+ mockUmRemoveUserOrSetEphemeral(mMockedUserManager, user, evenWhenDisallowed, result,
+ /* listener= */ null);
+ }
+
private void mockHalGetInitialInfo(@UserIdInt int currentUserId,
@NonNull InitialUserInfoResponse response) {
UsersInfo usersInfo = newUsersInfo(currentUserId);
@@ -2456,7 +2473,7 @@
}
private void verifyNoUserRemoved() {
- verify(mMockedUserManager, never()).removeUserOrSetEphemeral(anyInt());
+ verify(mMockedUserManager, never()).removeUserOrSetEphemeral(anyInt(), anyBoolean());
verify(mMockedUserManager, never()).removeUser(anyInt());
}
@@ -2582,7 +2599,12 @@
}
private void assertHalRemove(@NonNull UserInfo currentUser, @NonNull UserInfo removeUser) {
- verify(mMockedUserManager).removeUserOrSetEphemeral(removeUser.id);
+ assertHalRemove(currentUser, removeUser, /* evenWhenDisallowed= */ false);
+ }
+
+ private void assertHalRemove(@NonNull UserInfo currentUser, @NonNull UserInfo removeUser,
+ boolean evenWhenDisallowed) {
+ verify(mMockedUserManager).removeUserOrSetEphemeral(removeUser.id, evenWhenDisallowed);
ArgumentCaptor<RemoveUserRequest> requestCaptor =
ArgumentCaptor.forClass(RemoveUserRequest.class);
verify(mUserHal).removeUser(requestCaptor.capture());
@@ -2592,6 +2614,16 @@
assertThat(request.usersInfo.currentUser.userId).isEqualTo(currentUser.id);
}
+ private void assertUserRemovalResultStatus(UserRemovalResult result,
+ @UserRemovalResult.Status int expectedStatus) {
+ int actualStatus = result.getStatus();
+ if (actualStatus != expectedStatus) {
+ fail(String.format("wrong UserRemovalResult: expected %s, got %s",
+ UserRemovalResult.statusToString(expectedStatus),
+ UserRemovalResult.statusToString(actualStatus)));
+ }
+ }
+
@NonNull
private static SwitchUserRequest isSwitchUserRequest(int requestId,
@UserIdInt int currentUserId, @UserIdInt int targetUserId) {
