Merge "Added sanity checks on CarUserManager listener management methods." into rvc-dev
diff --git a/car-lib/src/android/car/user/CarUserManager.java b/car-lib/src/android/car/user/CarUserManager.java
index 784ee48..7daf63b 100644
--- a/car-lib/src/android/car/user/CarUserManager.java
+++ b/car-lib/src/android/car/user/CarUserManager.java
@@ -55,6 +55,7 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.util.List;
+import java.util.Objects;
import java.util.concurrent.Executor;
/**
@@ -231,6 +232,8 @@
/**
* Adds a listener for {@link UserLifecycleEvent user lifecycle events}.
*
+ * @throws IllegalStateException if the listener was already added.
+ *
* @hide
*/
@SystemApi
@@ -238,15 +241,14 @@
@RequiresPermission(anyOf = {INTERACT_ACROSS_USERS, INTERACT_ACROSS_USERS_FULL})
public void addListener(@NonNull @CallbackExecutor Executor executor,
@NonNull UserLifecycleListener listener) {
+ Objects.requireNonNull(executor, "executor cannot be null");
+ Objects.requireNonNull(listener, "listener cannot be null");
checkInteractAcrossUsersPermission();
- // TODO(b/144120654): add unit tests to validate input
- // - executor cannot be null
- // - listener cannot be null
- // - listener must not be added before
-
int uid = myUid();
synchronized (mLock) {
+ Preconditions.checkState(mListeners == null || !mListeners.containsKey(listener),
+ "already called for this listener");
if (mReceiver == null) {
mReceiver = new LifecycleResultReceiver();
try {
@@ -271,24 +273,21 @@
/**
* Removes a listener for {@link UserLifecycleEvent user lifecycle events}.
*
+ * @throws IllegalStateException if the listener was not added beforehand.
+ *
* @hide
*/
@SystemApi
@TestApi
@RequiresPermission(anyOf = {INTERACT_ACROSS_USERS, INTERACT_ACROSS_USERS_FULL})
public void removeListener(@NonNull UserLifecycleListener listener) {
+ Objects.requireNonNull(listener, "listener cannot be null");
checkInteractAcrossUsersPermission();
- // TODO(b/144120654): add unit tests to validate input
- // - listener cannot be null
- // - listener must not be added before
int uid = myUid();
synchronized (mLock) {
- if (mListeners == null) {
- Log.w(TAG, "removeListener(): no listeners for uid " + uid);
- return;
- }
-
+ Preconditions.checkState(mListeners != null && mListeners.containsKey(listener),
+ "not called for this listener yet");
mListeners.remove(listener);
if (!mListeners.isEmpty()) {
diff --git a/tests/carservice_unit_test/src/com/android/car/user/CarUserManagerUnitTest.java b/tests/carservice_unit_test/src/com/android/car/user/CarUserManagerUnitTest.java
index abfd48d..5a93b8c 100644
--- a/tests/carservice_unit_test/src/com/android/car/user/CarUserManagerUnitTest.java
+++ b/tests/carservice_unit_test/src/com/android/car/user/CarUserManagerUnitTest.java
@@ -15,9 +15,11 @@
*/
package com.android.car.user;
+import static android.Manifest.permission.INTERACT_ACROSS_USERS;
import static android.car.test.mocks.AndroidMockitoHelper.mockUmGetUsers;
import static android.car.test.util.UserTestingHelper.newUsers;
import static android.car.testapi.CarMockitoHelper.mockHandleRemoteExceptionFromCarServiceWithDefaultValue;
+import static android.content.pm.PackageManager.PERMISSION_GRANTED;
import static android.os.UserHandle.USER_SYSTEM;
import static com.google.common.truth.Truth.assertThat;
@@ -28,6 +30,7 @@
import static org.mockito.ArgumentMatchers.notNull;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.testng.Assert.assertThrows;
@@ -39,9 +42,11 @@
import android.car.ICarUserService;
import android.car.test.mocks.AbstractExtendedMockitoTestCase;
import android.car.user.CarUserManager;
+import android.car.user.CarUserManager.UserLifecycleListener;
import android.car.user.CarUserManager.UserSwitchUiCallback;
import android.car.user.GetUserIdentificationAssociationResponse;
import android.car.user.UserSwitchResult;
+import android.content.Context;
import android.content.pm.UserInfo;
import android.os.RemoteException;
import android.os.UserManager;
@@ -117,9 +122,68 @@
}
@Test
+ public void testAddListener_nullExecutor() {
+ mockInteractAcrossUsersPermission();
+
+ assertThrows(NullPointerException.class, () -> mMgr.addListener(null, (e) -> { }));
+ }
+
+ @Test
+ public void testAddListener_nullListener() {
+ mockInteractAcrossUsersPermission();
+
+ assertThrows(NullPointerException.class, () -> mMgr.addListener(Runnable::run, null));
+ }
+
+ @Test
+ public void testAddListener_sameListenerAddedTwice() {
+ mockInteractAcrossUsersPermission();
+
+ UserLifecycleListener listener = (e) -> { };
+
+ mMgr.addListener(Runnable::run, listener);
+ assertThrows(IllegalStateException.class, () -> mMgr.addListener(Runnable::run, listener));
+ }
+
+ @Test
+ public void testAddListener_differentListenersAddedTwice() {
+ mockInteractAcrossUsersPermission();
+
+ mMgr.addListener(Runnable::run, (e) -> { });
+ mMgr.addListener(Runnable::run, (e) -> { });
+ }
+
+ @Test
+ public void testRemoveListener_nullListener() {
+ mockInteractAcrossUsersPermission();
+
+ assertThrows(NullPointerException.class, () -> mMgr.removeListener(null));
+ }
+
+ @Test
+ public void testRemoveListener_notAddedBefore() {
+ mockInteractAcrossUsersPermission();
+
+ UserLifecycleListener listener = (e) -> { };
+
+ assertThrows(IllegalStateException.class, () -> mMgr.removeListener(listener));
+ }
+
+ @Test
+ public void testRemoveListener_addAndRemove() {
+ mockInteractAcrossUsersPermission();
+ UserLifecycleListener listener = (e) -> { };
+
+ mMgr.addListener(Runnable::run, listener);
+ mMgr.removeListener(listener);
+
+ // Make sure it was removed
+ assertThrows(IllegalStateException.class, () -> mMgr.removeListener(listener));
+ }
+
+ @Test
public void testSwitchUser_success() throws Exception {
- expectServiceSwitchUserSucceeds(11, UserSwitchResult.STATUS_SUCCESSFUL,
- "D'OH!");
+ expectServiceSwitchUserSucceeds(11, UserSwitchResult.STATUS_SUCCESSFUL, "D'OH!");
AndroidFuture<UserSwitchResult> future = mMgr.switchUser(11);
@@ -188,6 +252,13 @@
assertThat(actualResponse).isSameAs(expectedResponse);
}
+ // TODO(b/155311595): remove once permission check is done only on service
+ private void mockInteractAcrossUsersPermission() {
+ Context context = mock(Context.class);
+ when(mCar.getContext()).thenReturn(context);
+ when(context.checkSelfPermission(INTERACT_ACROSS_USERS)).thenReturn(PERMISSION_GRANTED);
+ }
+
private void expectServiceSwitchUserSucceeds(@UserIdInt int userId,
@UserSwitchResult.Status int status, @Nullable String errorMessage)
throws RemoteException {