Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a88a8477 authored by Soonil Nagarkar's avatar Soonil Nagarkar
Browse files

Fix bugs in AppOp restrictions 

1) Ensure restrictions are refreshed when provider identities change.
2) Only calculate exempt packages when restrictions are applied.

Bug: 187421886
Test: manual
Change-Id: I6f639d24944e1350215969835875d2d227cb6cb6
parent 28c48606

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

+60 −20
Original line number Diff line number Diff line
@@ -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.setOnProviderLocationTagsChangeListener(

                    mOnProviderLocationTagsChangeListener);

            manager.startManager(this);



            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);





        boolean enabled = mInjector.getSettingsHelper().isLocationEnabled(userId);



        String[] allowedPackages = null;

        if (!enabled) {

            ArraySet<String> packages = new ArraySet<>();

            for (LocationProviderManager manager : mProviderManagers) {

            packages.add(manager.getIdentity().getPackageName());

                CallerIdentity identity = manager.getIdentity();

                if (identity != null) {

                    packages.add(identity.getPackageName());

                }

            }

            packages.add(mContext.getPackageName());

            packages.addAll(mInjector.getSettingsHelper().getIgnoreSettingsPackageWhitelist());

        String[] allowedPackages = packages.toArray(new String[0]);



        boolean enabled = mInjector.getSettingsHelper().isLocationEnabled(userId);

            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

+13 −44
Original line number Diff line number Diff line
@@ -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 (!oldState.extraAttributionTags.equals(newState.extraAttributionTags)

                    || !Objects.equals(oldState.identity, newState.identity)) {

                if (oldState.identity != null) {

                    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)

                    ));

                }

            }

        if (mStateChangedListener != null) {

            StateChangedListener listener = mStateChangedListener;

            FgThread.getExecutor().execute(

                    () -> listener.onStateChanged(mName, oldState, newState));

        }

    }

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

+13 −13
Original line number Diff line number Diff line
@@ -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() {

        OnProviderLocationTagsChangeListener listener = mock(

                OnProviderLocationTagsChangeListener.class);

        mManager.setOnProviderLocationTagsChangeListener(listener);



    public void testStateChangedListener() {

        mProvider.setExtraAttributionTags(Collections.singleton("extra"));



        ArgumentCaptor<LocationTagInfo> captor = ArgumentCaptor.forClass(LocationTagInfo.class);

        verify(listener, times(2)).onLocationTagsChanged(captor.capture());

        ArgumentCaptor<AbstractLocationProvider.State> captorOld = ArgumentCaptor.forClass(

                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(captor.getAllValues().get(1).getTags()).containsExactly("extra", "attribution");

        assertThat(captorOld.getAllValues().get(1).extraAttributionTags).isEmpty();

        assertThat(captorNew.getAllValues().get(1).extraAttributionTags).containsExactly("extra");

    }



    @Test