Merge "Fix bugs in AppOp restrictions" into sc-dev

services/core/java/com/android/server/location/LocationManagerService.java

@@ -146,7 +146,8 @@ import java.util.concurrent.CopyOnWriteArrayList;
  * The service class that manages LocationProviders and issues location
  * updates and alerts.
  */
-public class LocationManagerService extends ILocationManager.Stub {
+public class LocationManagerService extends ILocationManager.Stub implements
+        LocationProviderManager.StateChangedListener {
 
     /**
      * Controls lifecycle of LocationManagerService.
@@ -228,7 +229,7 @@ public class LocationManagerService extends ILocationManager.Stub {
 
     private static final String ATTRIBUTION_TAG = "LocationService";
 
-    private final Object mLock = new Object();
+    final Object mLock = new Object();
 
     private final Context mContext;
     private final Injector mInjector;
@@ -257,7 +258,7 @@ public class LocationManagerService extends ILocationManager.Stub {
             new CopyOnWriteArrayList<>();
 
     @GuardedBy("mLock")
-    private @Nullable OnProviderLocationTagsChangeListener mOnProviderLocationTagsChangeListener;
+    @Nullable OnProviderLocationTagsChangeListener mOnProviderLocationTagsChangeListener;
 
     LocationManagerService(Context context, Injector injector) {
         mContext = context.createAttributionContext(ATTRIBUTION_TAG);
@@ -331,9 +332,8 @@ public class LocationManagerService extends ILocationManager.Stub {
         synchronized (mProviderManagers) {
             Preconditions.checkState(getLocationProviderManager(manager.getName()) == null);
 
-            manager.startManager();
+            manager.startManager(this);
-            manager.setOnProviderLocationTagsChangeListener(
+
-                    mOnProviderLocationTagsChangeListener);
             if (realProvider != null) {
                 // custom logic wrapping all non-passive providers
                 if (manager != mPassiveManager) {
@@ -1356,6 +1356,44 @@ public class LocationManagerService extends ILocationManager.Stub {
         ipw.decreaseIndent();
     }
 
+    @Override
+    public void onStateChanged(String provider, AbstractLocationProvider.State oldState,
+            AbstractLocationProvider.State newState) {
+        if (!Objects.equals(oldState.identity, newState.identity)) {
+            refreshAppOpsRestrictions(UserHandle.USER_ALL);
+        }
+
+        OnProviderLocationTagsChangeListener listener;
+        synchronized (mLock) {
+            listener = mOnProviderLocationTagsChangeListener;
+        }
+
+        if (listener != null) {
+            if (!oldState.extraAttributionTags.equals(newState.extraAttributionTags)
+                    || !Objects.equals(oldState.identity, newState.identity)) {
+                if (oldState.identity != null) {
+                    listener.onLocationTagsChanged(
+                            new LocationManagerInternal.LocationTagInfo(
+                                    oldState.identity.getUid(),
+                                    oldState.identity.getPackageName(),
+                                    Collections.emptySet()));
+                }
+                if (newState.identity != null) {
+                    ArraySet<String> attributionTags = new ArraySet<>(
+                            newState.extraAttributionTags.size() + 1);
+                    attributionTags.addAll(newState.extraAttributionTags);
+                    attributionTags.add(newState.identity.getAttributionTag());
+
+                    listener.onLocationTagsChanged(
+                            new LocationManagerInternal.LocationTagInfo(
+                                    newState.identity.getUid(),
+                                    newState.identity.getPackageName(),
+                                    attributionTags));
+                }
+            }
+        }
+    }
+
     private void refreshAppOpsRestrictions(int userId) {
         if (userId == UserHandle.USER_ALL) {
             final int[] runningUserIds = mInjector.getUserInfoHelper().getRunningUserIds();
@@ -1367,27 +1405,34 @@ public class LocationManagerService extends ILocationManager.Stub {
 
         Preconditions.checkArgument(userId >= 0);
 
-        ArraySet<String> packages = new ArraySet<>();
-        for (LocationProviderManager manager : mProviderManagers) {
-            packages.add(manager.getIdentity().getPackageName());
-        }
-        packages.add(mContext.getPackageName());
-        packages.addAll(mInjector.getSettingsHelper().getIgnoreSettingsPackageWhitelist());
-        String[] allowedPackages = packages.toArray(new String[0]);
 
         boolean enabled = mInjector.getSettingsHelper().isLocationEnabled(userId);
 
+        String[] allowedPackages = null;
+        if (!enabled) {
+            ArraySet<String> packages = new ArraySet<>();
+            for (LocationProviderManager manager : mProviderManagers) {
+                CallerIdentity identity = manager.getIdentity();
+                if (identity != null) {
+                    packages.add(identity.getPackageName());
+                }
+            }
+            packages.add(mContext.getPackageName());
+            packages.addAll(mInjector.getSettingsHelper().getIgnoreSettingsPackageWhitelist());
+            allowedPackages = packages.toArray(new String[0]);
+        }
+
         AppOpsManager appOpsManager = Objects.requireNonNull(
                 mContext.getSystemService(AppOpsManager.class));
         appOpsManager.setUserRestrictionForUser(
                 AppOpsManager.OP_COARSE_LOCATION,
-                enabled,
+                !enabled,
                 LocationManagerService.this,
                 allowedPackages,
                 userId);
         appOpsManager.setUserRestrictionForUser(
                 AppOpsManager.OP_FINE_LOCATION,
-                enabled,
+                !enabled,
                 LocationManagerService.this,
                 allowedPackages,
                 userId);
@@ -1467,11 +1512,6 @@ public class LocationManagerService extends ILocationManager.Stub {
                 @Nullable OnProviderLocationTagsChangeListener listener) {
             synchronized (mLock) {
                 mOnProviderLocationTagsChangeListener = listener;
-                final int providerCount = mProviderManagers.size();
-                for (int i = 0; i < providerCount; i++) {
-                    final LocationProviderManager manager = mProviderManagers.get(i);
-                    manager.setOnProviderLocationTagsChangeListener(listener);
-                }
             }
         }
     }

services/core/java/com/android/server/location/provider/LocationProviderManager.java

@@ -55,8 +55,6 @@ import android.location.LastLocationRequest;
 import android.location.Location;
 import android.location.LocationManager;
 import android.location.LocationManagerInternal;
-import android.location.LocationManagerInternal.LocationTagInfo;
-import android.location.LocationManagerInternal.OnProviderLocationTagsChangeListener;
 import android.location.LocationManagerInternal.ProviderEnabledListener;
 import android.location.LocationRequest;
 import android.location.LocationResult;
@@ -90,7 +88,6 @@ import android.util.TimeUtils;
 
 import com.android.internal.annotations.GuardedBy;
 import com.android.internal.util.Preconditions;
-import com.android.internal.util.function.pooled.PooledLambda;
 import com.android.server.FgThread;
 import com.android.server.LocalServices;
 import com.android.server.location.LocationPermissions;
@@ -122,7 +119,6 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.Objects;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.function.Predicate;
@@ -171,6 +167,11 @@ public class LocationProviderManager extends
     private static final int STATE_STOPPING = 1;
     private static final int STATE_STOPPED = 2;
 
+    public interface StateChangedListener {
+        void onStateChanged(String provider, AbstractLocationProvider.State oldState,
+                AbstractLocationProvider.State newState);
+    }
+
     protected interface LocationTransport {
 
         void deliverOnLocationChanged(LocationResult locationResult,
@@ -1315,7 +1316,7 @@ public class LocationProviderManager extends
     private @Nullable OnAlarmListener mDelayedRegister;
 
     @GuardedBy("mLock")
-    private @Nullable OnProviderLocationTagsChangeListener mOnLocationTagsChangeListener;
+    private @Nullable StateChangedListener mStateChangedListener;
 
     public LocationProviderManager(Context context, Injector injector,
             String name, @Nullable PassiveLocationProviderManager passiveManager) {
@@ -1354,10 +1355,11 @@ public class LocationProviderManager extends
         return TAG;
     }
 
-    public void startManager() {
+    public void startManager(@Nullable StateChangedListener listener) {
         synchronized (mLock) {
             Preconditions.checkState(mState == STATE_STOPPED);
             mState = STATE_STARTED;
+            mStateChangedListener = listener;
 
             mUserHelper.addListener(mUserChangedListener);
             mSettingsHelper.addOnLocationEnabledChangedListener(mLocationEnabledChangedListener);
@@ -1396,6 +1398,7 @@ public class LocationProviderManager extends
 
             mEnabled.clear();
             mLastLocations.clear();
+            mStateChangedListener = null;
             mState = STATE_STOPPED;
         }
     }
@@ -1476,19 +1479,6 @@ public class LocationProviderManager extends
         }
     }
 
-    /**
-     * Registers a listener for the location tags of the provider.
-     *
-     * @param listener The listener
-     */
-    public void setOnProviderLocationTagsChangeListener(
-            @Nullable OnProviderLocationTagsChangeListener listener) {
-        Preconditions.checkArgument(mOnLocationTagsChangeListener == null || listener == null);
-        synchronized (mLock) {
-            mOnLocationTagsChangeListener = listener;
-        }
-    }
-
     public void setMockProvider(@Nullable MockLocationProvider provider) {
         synchronized (mLock) {
             Preconditions.checkState(mState != STATE_STOPPED);
@@ -2292,31 +2282,10 @@ public class LocationProviderManager extends
             updateRegistrations(Registration::onProviderPropertiesChanged);
         }
 
-        if (mOnLocationTagsChangeListener != null) {
+        if (mStateChangedListener != null) {
-            if (!oldState.extraAttributionTags.equals(newState.extraAttributionTags)
+            StateChangedListener listener = mStateChangedListener;
-                    || !Objects.equals(oldState.identity, newState.identity)) {
+            FgThread.getExecutor().execute(
-                if (oldState.identity != null) {
+                    () -> listener.onStateChanged(mName, oldState, newState));
-                    FgThread.getHandler().sendMessage(PooledLambda.obtainMessage(
-                            OnProviderLocationTagsChangeListener::onLocationTagsChanged,
-                            mOnLocationTagsChangeListener, new LocationTagInfo(
-                                    oldState.identity.getUid(), oldState.identity.getPackageName(),
-                                    Collections.emptySet())
-                    ));
-                }
-                if (newState.identity != null) {
-                    ArraySet<String> attributionTags = new ArraySet<>(
-                            newState.extraAttributionTags.size() + 1);
-                    attributionTags.addAll(newState.extraAttributionTags);
-                    attributionTags.add(newState.identity.getAttributionTag());
-
-                    FgThread.getHandler().sendMessage(PooledLambda.obtainMessage(
-                            OnProviderLocationTagsChangeListener::onLocationTagsChanged,
-                            mOnLocationTagsChangeListener, new LocationTagInfo(
-                                    newState.identity.getUid(), newState.identity.getPackageName(),
-                                    attributionTags)
-                    ));
-                }
-            }
         }
     }
 

services/tests/mockingservicestests/src/com/android/server/location/provider/LocationProviderManagerTest.java

@@ -61,8 +61,6 @@ import android.location.ILocationListener;
 import android.location.LastLocationRequest;
 import android.location.Location;
 import android.location.LocationManagerInternal;
-import android.location.LocationManagerInternal.LocationTagInfo;
-import android.location.LocationManagerInternal.OnProviderLocationTagsChangeListener;
 import android.location.LocationManagerInternal.ProviderEnabledListener;
 import android.location.LocationRequest;
 import android.location.LocationResult;
@@ -133,6 +131,8 @@ public class LocationProviderManagerTest {
 
     private Random mRandom;
 
+    @Mock
+    private LocationProviderManager.StateChangedListener mStateChangedListener;
     @Mock
     private LocationManagerInternal mInternal;
     @Mock
@@ -167,14 +167,14 @@ public class LocationProviderManagerTest {
         mInjector.getUserInfoHelper().startUser(OTHER_USER);
 
         mPassive = new PassiveLocationProviderManager(mContext, mInjector);
-        mPassive.startManager();
+        mPassive.startManager(null);
         mPassive.setRealProvider(new PassiveLocationProvider(mContext));
 
         mProvider = new TestProvider(PROPERTIES, PROVIDER_IDENTITY);
         mProvider.setProviderAllowed(true);
 
         mManager = new LocationProviderManager(mContext, mInjector, NAME, mPassive);
-        mManager.startManager();
+        mManager.startManager(mStateChangedListener);
         mManager.setRealProvider(mProvider);
     }
 
@@ -219,18 +219,18 @@ public class LocationProviderManagerTest {
     }
 
     @Test
-    public void testAttributionTags() {
+    public void testStateChangedListener() {
-        OnProviderLocationTagsChangeListener listener = mock(
-                OnProviderLocationTagsChangeListener.class);
-        mManager.setOnProviderLocationTagsChangeListener(listener);
-
         mProvider.setExtraAttributionTags(Collections.singleton("extra"));
 
-        ArgumentCaptor<LocationTagInfo> captor = ArgumentCaptor.forClass(LocationTagInfo.class);
+        ArgumentCaptor<AbstractLocationProvider.State> captorOld = ArgumentCaptor.forClass(
-        verify(listener, times(2)).onLocationTagsChanged(captor.capture());
+                AbstractLocationProvider.State.class);
+        ArgumentCaptor<AbstractLocationProvider.State> captorNew = ArgumentCaptor.forClass(
+                AbstractLocationProvider.State.class);
+        verify(mStateChangedListener, timeout(TIMEOUT_MS).times(2)).onStateChanged(eq(NAME),
+                captorOld.capture(), captorNew.capture());
 
-        assertThat(captor.getAllValues().get(0).getTags()).isEmpty();
+        assertThat(captorOld.getAllValues().get(1).extraAttributionTags).isEmpty();
-        assertThat(captor.getAllValues().get(1).getTags()).containsExactly("extra", "attribution");
+        assertThat(captorNew.getAllValues().get(1).extraAttributionTags).containsExactly("extra");
     }
 
     @Test