Merge "Fix bugs in LocationManagerService and AppOpsPolicy" into sc-dev

9 files changed, 350 insertions, 238 deletions

diff --git a/core/java/android/os/PackageTagsList.java b/core/java/android/os/PackageTagsList.java
index c94d3de33b6f..4a81dc6f592e 100644
--- a/core/java/android/os/PackageTagsList.java
+++ b/core/java/android/os/PackageTagsList.java
@@ -23,20 +23,26 @@ import android.annotation.TestApi;
 import android.util.ArrayMap;
 import android.util.ArraySet;
 
+import com.android.internal.annotations.Immutable;
+
 import java.io.PrintWriter;
+import java.util.Collection;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 
 /**
- * A list of packages and associated attribution tags that supports easy membership checks.
+ * A list of packages and associated attribution tags that supports easy membership checks. Supports
+ * "wildcard" attribution tags (ie, matching any attribution tag under a package) in additional to
+ * standard checks.
  *
  * @hide
  */
 @TestApi
+@Immutable
 public final class PackageTagsList implements Parcelable {
 
-    // an empty set value matches any attribution tag
+    // an empty set value matches any attribution tag (ie, wildcard)
     private final ArrayMap<String, ArraySet<String>> mPackageTags;
 
     private PackageTagsList(@NonNull ArrayMap<String, ArraySet<String>> packageTags) {
@@ -51,15 +57,34 @@ public final class PackageTagsList implements Parcelable {
     }
 
     /**
-     * Returns true if the given package is represented within this instance. If this returns true
-     * this does not imply anything about whether any given attribution tag under the given package
-     * name is present.
+     * Returns true if the given package is found within this instance. If this returns true this
+     * does not imply anything about whether any given attribution tag under the given package name
+     * is present.
      */
     public boolean includes(@NonNull String packageName) {
         return mPackageTags.containsKey(packageName);
     }
 
     /**
+     * Returns true if the given attribution tag is found within this instance under any package.
+     * Only returns true if the attribution tag literal is found, not if any package contains the
+     * set of all attribution tags.
+     *
+     * @hide
+     */
+    public boolean includesTag(@NonNull String attributionTag) {
+        final int size = mPackageTags.size();
+        for (int i = 0; i < size; i++) {
+            ArraySet<String> tags = mPackageTags.valueAt(i);
+            if (tags.contains(attributionTag)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    /**
      * Returns true if all attribution tags under the given package are contained within this
      * instance.
      */
@@ -76,6 +101,7 @@ public final class PackageTagsList implements Parcelable {
         if (tags == null) {
             return false;
         } else if (tags.isEmpty()) {
+            // our tags are the full set, so we contain any attribution tag
             return true;
         } else {
             return tags.contains(attributionTag);
@@ -98,10 +124,12 @@ public final class PackageTagsList implements Parcelable {
                 return false;
             }
             if (tags.isEmpty()) {
+                // our tags are the full set, so we contain whatever the other tags are
                 continue;
             }
             ArraySet<String> otherTags = packageTagsList.mPackageTags.valueAt(i);
             if (otherTags.isEmpty()) {
+                // other tags are the full set, so we can't contain them
                 return false;
             }
             if (!tags.containsAll(otherTags)) {
@@ -248,6 +276,31 @@ public final class PackageTagsList implements Parcelable {
         }
 
         /**
+         * Adds the specified package and set of attribution tags to the builder.
+         *
+         * @hide
+         */
+        @SuppressLint("MissingGetterMatchingBuilder")
+        public @NonNull Builder add(@NonNull String packageName,
+                @NonNull Collection<String> attributionTags) {
+            if (attributionTags.isEmpty()) {
+                // the input is not allowed to specify a full set by passing in an empty collection
+                return this;
+            }
+
+            ArraySet<String> tags = mPackageTags.get(packageName);
+            if (tags == null) {
+                tags = new ArraySet<>(attributionTags);
+                mPackageTags.put(packageName, tags);
+            } else if (!tags.isEmpty()) {
+                // if we contain the full set, already done, otherwise add all the tags
+                tags.addAll(attributionTags);
+            }
+
+            return this;
+        }
+
+        /**
          * Adds the specified {@link PackageTagsList} to the builder.
          */
         @SuppressLint("MissingGetterMatchingBuilder")
@@ -267,13 +320,92 @@ public final class PackageTagsList implements Parcelable {
                 if (newTags.isEmpty()) {
                     add(entry.getKey());
                 } else {
-                    ArraySet<String> tags = mPackageTags.get(entry.getKey());
-                    if (tags == null) {
-                        tags = new ArraySet<>(newTags);
-                        mPackageTags.put(entry.getKey(), tags);
-                    } else if (!tags.isEmpty()) {
-                        tags.addAll(newTags);
-                    }
+                    add(entry.getKey(), newTags);
+                }
+            }
+
+            return this;
+        }
+
+        /**
+         * Removes all attribution tags under the specified package from the builder.
+         *
+         * @hide
+         */
+        @SuppressLint("MissingGetterMatchingBuilder")
+        public @NonNull Builder remove(@NonNull String packageName) {
+            mPackageTags.remove(packageName);
+            return this;
+        }
+
+        /**
+         * Removes the specified package and attribution tag from the builder if and only if the
+         * specified attribution tag is listed explicitly under the package. If the package contains
+         * all possible attribution tags, then nothing will be removed.
+         *
+         * @hide
+         */
+        @SuppressLint("MissingGetterMatchingBuilder")
+        public @NonNull Builder remove(@NonNull String packageName,
+                @Nullable String attributionTag) {
+            ArraySet<String> tags = mPackageTags.get(packageName);
+            if (tags != null && tags.remove(attributionTag) && tags.isEmpty()) {
+                mPackageTags.remove(packageName);
+            }
+            return this;
+        }
+
+        /**
+         * Removes the specified package and set of attribution tags from the builder if and only if
+         * the specified set of attribution tags are listed explicitly under the package. If the
+         * package contains all possible attribution tags, then nothing will be removed.
+         *
+         * @hide
+         */
+        @SuppressLint("MissingGetterMatchingBuilder")
+        public @NonNull Builder remove(@NonNull String packageName,
+                @NonNull Collection<String> attributionTags) {
+            if (attributionTags.isEmpty()) {
+                // the input is not allowed to specify a full set by passing in an empty collection
+                return this;
+            }
+
+            ArraySet<String> tags = mPackageTags.get(packageName);
+            if (tags != null && tags.removeAll(attributionTags) && tags.isEmpty()) {
+                mPackageTags.remove(packageName);
+            }
+            return this;
+        }
+
+        /**
+         * Removes the specified {@link PackageTagsList} from the builder. If a package contains all
+         * possible attribution tags, it will only be removed if the package in the removed
+         * {@link PackageTagsList} also contains all possible attribution tags.
+         *
+         * @hide
+         */
+        @SuppressLint("MissingGetterMatchingBuilder")
+        public @NonNull Builder remove(@NonNull PackageTagsList packageTagsList) {
+            return remove(packageTagsList.mPackageTags);
+        }
+
+        /**
+         * Removes the given map of package to attribution tags to the builder. An empty set of
+         * attribution tags is interpreted to imply all attribution tags under that package. If a
+         * package contains all possible attribution tags, it will only be removed if the package in
+         * the removed map also contains all possible attribution tags.
+         *
+         * @hide
+         */
+        @SuppressLint("MissingGetterMatchingBuilder")
+        public @NonNull Builder remove(@NonNull Map<String, ? extends Set<String>> packageTagsMap) {
+            for (Map.Entry<String, ? extends Set<String>> entry : packageTagsMap.entrySet()) {
+                Set<String> removedTags = entry.getValue();
+                if (removedTags.isEmpty()) {
+                    // if removing the full set, drop the package completely
+                    remove(entry.getKey());
+                } else {
+                    remove(entry.getKey(), removedTags);
                 }
             }
 
diff --git a/core/tests/coretests/src/android/os/PackageTagsListTest.java b/core/tests/coretests/src/android/os/PackageTagsListTest.java
index 518e02e44b06..9034a5ccdd7f 100644
--- a/core/tests/coretests/src/android/os/PackageTagsListTest.java
+++ b/core/tests/coretests/src/android/os/PackageTagsListTest.java
@@ -30,6 +30,7 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import java.util.Arrays;
+import java.util.Collections;
 
 @Presubmit
 @RunWith(AndroidJUnit4.class)
@@ -40,7 +41,8 @@ public class PackageTagsListTest {
         PackageTagsList.Builder builder = new PackageTagsList.Builder()
                 .add("package1", "attr1")
                 .add("package1", "attr2")
-                .add("package2");
+                .add("package2")
+                .add("package4", Arrays.asList("attr1", "attr2"));
         PackageTagsList list = builder.build();
 
         assertTrue(list.contains(builder.build()));
@@ -49,10 +51,13 @@ public class PackageTagsListTest {
         assertTrue(list.contains("package2", "attr1"));
         assertTrue(list.contains("package2", "attr2"));
         assertTrue(list.contains("package2", "attr3"));
+        assertTrue(list.contains("package4", "attr1"));
+        assertTrue(list.contains("package4", "attr2"));
         assertTrue(list.containsAll("package2"));
         assertTrue(list.includes("package1"));
         assertTrue(list.includes("package2"));
         assertFalse(list.contains("package1", "attr3"));
+        assertFalse(list.contains("package4", "attr3"));
         assertFalse(list.containsAll("package1"));
         assertFalse(list.includes("package3"));
 
@@ -92,6 +97,51 @@ public class PackageTagsListTest {
     }
 
     @Test
+    public void testPackageTagsList_Remove() {
+        PackageTagsList.Builder builder = new PackageTagsList.Builder()
+                .add("package1", "attr1")
+                .add("package1", "attr2")
+                .add("package2")
+                .add("package4", Arrays.asList("attr1", "attr2", "attr3"))
+                .add("package3", "attr1")
+                .remove("package1", "attr1")
+                .remove("package1", "attr2")
+                .remove("package2", "attr1")
+                .remove("package4", Arrays.asList("attr1", "attr2"))
+                .remove("package3");
+        PackageTagsList list = builder.build();
+
+        assertTrue(list.contains(builder.build()));
+        assertFalse(list.contains("package1", "attr1"));
+        assertFalse(list.contains("package1", "attr2"));
+        assertTrue(list.contains("package2", "attr1"));
+        assertTrue(list.contains("package2", "attr2"));
+        assertTrue(list.contains("package2", "attr3"));
+        assertFalse(list.contains("package3", "attr1"));
+        assertFalse(list.contains("package4", "attr1"));
+        assertFalse(list.contains("package4", "attr2"));
+        assertTrue(list.contains("package4", "attr3"));
+        assertTrue(list.containsAll("package2"));
+        assertFalse(list.includes("package1"));
+        assertTrue(list.includes("package2"));
+        assertFalse(list.includes("package3"));
+        assertTrue(list.includes("package4"));
+    }
+
+    @Test
+    public void testPackageTagsList_EmptyCollections() {
+        PackageTagsList.Builder builder = new PackageTagsList.Builder()
+                .add("package1", Collections.emptyList())
+                .add("package2")
+                .remove("package2", Collections.emptyList());
+        PackageTagsList list = builder.build();
+
+        assertTrue(list.contains(builder.build()));
+        assertFalse(list.contains("package1", "attr1"));
+        assertTrue(list.contains("package2", "attr2"));
+    }
+
+    @Test
     public void testWriteToParcel() {
         PackageTagsList list = new PackageTagsList.Builder()
                 .add("package1", "attr1")
diff --git a/location/java/android/location/LocationManagerInternal.java b/location/java/android/location/LocationManagerInternal.java
index 763835c9cbe2..d59756d02348 100644
--- a/location/java/android/location/LocationManagerInternal.java
+++ b/location/java/android/location/LocationManagerInternal.java
@@ -16,14 +16,10 @@
 
 package android.location;
 
-
 import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.location.util.identity.CallerIdentity;
-
-import com.android.internal.annotations.Immutable;
-
-import java.util.Set;
+import android.os.PackageTagsList;
 
 /**
  * Location manager local system service interface.
@@ -43,18 +39,14 @@ public abstract class LocationManagerInternal {
     }
 
     /**
-     * Interface for getting callbacks when a location provider's location tags change.
-     *
-     * @see LocationTagInfo
+     * Interface for getting callbacks when an app id's location provider package tags change.
      */
-    public interface OnProviderLocationTagsChangeListener {
+    public interface LocationPackageTagsListener {
 
         /**
-         * Called when the location tags for a provider change.
-         *
-         * @param providerLocationTagInfo The tag info for a provider.
+         * Called when the package tags for a location provider change for a uid.
          */
-        void onLocationTagsChanged(@NonNull LocationTagInfo providerLocationTagInfo);
+        void onLocationPackageTagsChanged(int uid, @NonNull PackageTagsList packageTagsList);
     }
 
     /**
@@ -109,58 +101,9 @@ public abstract class LocationManagerInternal {
     public abstract @Nullable LocationTime getGnssTimeMillis();
 
     /**
-     * Sets a listener for changes in the location providers' tags. Passing
+     * Sets a listener for changes in an app id's location provider package tags. Passing
      * {@code null} clears the current listener.
-     *
-     * @param listener The listener.
      */
-    public abstract void setOnProviderLocationTagsChangeListener(
-            @Nullable OnProviderLocationTagsChangeListener listener);
-
-    /**
-     * This class represents the location permission tags used by the location provider
-     * packages in a given UID. These tags are strictly used for accessing state guarded
-     * by the location permission(s) by a location provider which are required for the
-     * provider to fulfill its function as being a location provider.
-     */
-    @Immutable
-    public static class LocationTagInfo {
-        private final int mUid;
-
-        @NonNull
-        private final String mPackageName;
-
-        @Nullable
-        private final Set<String> mLocationTags;
-
-        public LocationTagInfo(int uid, @NonNull String packageName,
-                @Nullable Set<String> locationTags) {
-            mUid = uid;
-            mPackageName = packageName;
-            mLocationTags = locationTags;
-        }
-
-        /**
-         * @return The UID for which tags are related.
-         */
-        public int getUid() {
-            return mUid;
-        }
-
-        /**
-         * @return The package for which tags are related.
-         */
-        @NonNull
-        public String getPackageName() {
-            return mPackageName;
-        }
-
-        /**
-         * @return The tags for the package used for location related accesses.
-         */
-        @Nullable
-        public Set<String> getTags() {
-            return mLocationTags;
-        }
-    }
+    public abstract void setLocationPackageTagsListener(
+            @Nullable LocationPackageTagsListener listener);
 }
diff --git a/services/core/java/com/android/server/location/LocationManagerService.java b/services/core/java/com/android/server/location/LocationManagerService.java
index b9e97e89051b..aeb1893c78b6 100644
--- a/services/core/java/com/android/server/location/LocationManagerService.java
+++ b/services/core/java/com/android/server/location/LocationManagerService.java
@@ -65,7 +65,7 @@ import android.location.LastLocationRequest;
 import android.location.Location;
 import android.location.LocationManager;
 import android.location.LocationManagerInternal;
-import android.location.LocationManagerInternal.OnProviderLocationTagsChangeListener;
+import android.location.LocationManagerInternal.LocationPackageTagsListener;
 import android.location.LocationProvider;
 import android.location.LocationRequest;
 import android.location.LocationTime;
@@ -93,6 +93,7 @@ import android.util.Log;
 import com.android.internal.annotations.GuardedBy;
 import com.android.internal.util.DumpUtils;
 import com.android.internal.util.Preconditions;
+import com.android.server.FgThread;
 import com.android.server.LocalServices;
 import com.android.server.SystemService;
 import com.android.server.location.eventlog.LocationEventLog;
@@ -259,7 +260,7 @@ public class LocationManagerService extends ILocationManager.Stub implements
             new CopyOnWriteArrayList<>();
 
     @GuardedBy("mLock")
-    @Nullable OnProviderLocationTagsChangeListener mOnProviderLocationTagsChangeListener;
+    @Nullable LocationPackageTagsListener mLocationTagsChangedListener;
 
     LocationManagerService(Context context, Injector injector) {
         mContext = context.createAttributionContext(ATTRIBUTION_TAG);
@@ -1363,32 +1364,28 @@ public class LocationManagerService extends ILocationManager.Stub implements
             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));
+        if (!oldState.extraAttributionTags.equals(newState.extraAttributionTags)
+                || !Objects.equals(oldState.identity, newState.identity)) {
+            // since we're potentially affecting the tag lists for two different uids, acquire the
+            // lock to ensure providers cannot change while we're looping over the providers
+            // multiple times, which could lead to inconsistent results.
+            synchronized (mLock) {
+                LocationPackageTagsListener listener = mLocationTagsChangedListener;
+                if (listener != null) {
+                    int oldUid = oldState.identity != null ? oldState.identity.getUid() : -1;
+                    int newUid = newState.identity != null ? newState.identity.getUid() : -1;
+                    if (oldUid != -1) {
+                        PackageTagsList tags = calculateAppOpsLocationSourceTags(oldUid);
+                        FgThread.getHandler().post(
+                                () -> listener.onLocationPackageTagsChanged(oldUid, tags));
+                    }
+                    // if the new app id is the same as the old app id, no need to invoke the
+                    // listener twice, it's already been taken care of
+                    if (newUid != -1 && newUid != oldUid) {
+                        PackageTagsList tags = calculateAppOpsLocationSourceTags(newUid);
+                        FgThread.getHandler().post(
+                                () -> listener.onLocationPackageTagsChanged(newUid, tags));
+                    }
                 }
             }
         }
@@ -1436,6 +1433,31 @@ public class LocationManagerService extends ILocationManager.Stub implements
                 userId);
     }
 
+    PackageTagsList calculateAppOpsLocationSourceTags(int uid) {
+        PackageTagsList.Builder builder = new PackageTagsList.Builder();
+        for (LocationProviderManager manager : mProviderManagers) {
+            AbstractLocationProvider.State managerState = manager.getState();
+            if (managerState.identity == null) {
+                continue;
+            }
+            if (managerState.identity.getUid() != uid) {
+                continue;
+            }
+
+            builder.add(managerState.identity.getPackageName(), managerState.extraAttributionTags);
+            if (managerState.extraAttributionTags.isEmpty()
+                    || managerState.identity.getAttributionTag() != null) {
+                builder.add(managerState.identity.getPackageName(),
+                        managerState.identity.getAttributionTag());
+            } else {
+                Log.e(TAG, manager.getName() + " provider has specified a null attribution tag and "
+                        + "a non-empty set of extra attribution tags - dropping the null "
+                        + "attribution tag");
+            }
+        }
+        return builder.build();
+    }
+
     private class LocalService extends LocationManagerInternal {
 
         LocalService() {}
@@ -1506,10 +1528,29 @@ public class LocationManagerService extends ILocationManager.Stub implements
         }
 
         @Override
-        public void setOnProviderLocationTagsChangeListener(
-                @Nullable OnProviderLocationTagsChangeListener listener) {
+        public void setLocationPackageTagsListener(
+                @Nullable LocationPackageTagsListener listener) {
             synchronized (mLock) {
-                mOnProviderLocationTagsChangeListener = listener;
+                mLocationTagsChangedListener = listener;
+
+                // calculate initial tag list and send to listener
+                if (listener != null) {
+                    ArraySet<Integer> uids = new ArraySet<>(mProviderManagers.size());
+                    for (LocationProviderManager manager : mProviderManagers) {
+                        CallerIdentity identity = manager.getIdentity();
+                        if (identity != null) {
+                            uids.add(identity.getUid());
+                        }
+                    }
+
+                    for (int uid : uids) {
+                        PackageTagsList tags = calculateAppOpsLocationSourceTags(uid);
+                        if (!tags.isEmpty()) {
+                            FgThread.getHandler().post(
+                                    () -> listener.onLocationPackageTagsChanged(uid, tags));
+                        }
+                    }
+                }
             }
         }
     }
diff --git a/services/core/java/com/android/server/location/provider/AbstractLocationProvider.java b/services/core/java/com/android/server/location/provider/AbstractLocationProvider.java
index 1da45bd49767..eb7b77a2234f 100644
--- a/services/core/java/com/android/server/location/provider/AbstractLocationProvider.java
+++ b/services/core/java/com/android/server/location/provider/AbstractLocationProvider.java
@@ -263,10 +263,10 @@ public abstract class AbstractLocationProvider {
     }
 
     /**
-     * The current allowed state of this provider.
+     * The current state of the provider.
      */
-    public final boolean isAllowed() {
-        return mInternalState.get().state.allowed;
+    public final State getState() {
+        return mInternalState.get().state;
     }
 
     /**
@@ -277,13 +277,6 @@ public abstract class AbstractLocationProvider {
     }
 
     /**
-     * The current provider properties of this provider.
-     */
-    public final @Nullable ProviderProperties getProperties() {
-        return mInternalState.get().state.properties;
-    }
-
-    /**
      * Call this method to report a change in provider properties.
      */
     protected void setProperties(@Nullable ProviderProperties properties) {
@@ -291,13 +284,6 @@ public abstract class AbstractLocationProvider {
     }
 
     /**
-     * The current identity of this provider.
-     */
-    public final @Nullable CallerIdentity getIdentity() {
-        return mInternalState.get().state.identity;
-    }
-
-    /**
      * Call this method to report a change in the provider's identity.
      */
     protected void setIdentity(@Nullable CallerIdentity identity) {
diff --git a/services/core/java/com/android/server/location/provider/LocationProviderManager.java b/services/core/java/com/android/server/location/provider/LocationProviderManager.java
index 4f8b87b51294..8d335b83d99c 100644
--- a/services/core/java/com/android/server/location/provider/LocationProviderManager.java
+++ b/services/core/java/com/android/server/location/provider/LocationProviderManager.java
@@ -1407,12 +1407,16 @@ public class LocationProviderManager extends
         return mName;
     }
 
+    public AbstractLocationProvider.State getState() {
+        return mProvider.getState();
+    }
+
     public @Nullable CallerIdentity getIdentity() {
-        return mProvider.getIdentity();
+        return mProvider.getState().identity;
     }
 
     public @Nullable ProviderProperties getProperties() {
-        return mProvider.getProperties();
+        return mProvider.getState().properties;
     }
 
     public boolean hasProvider() {
@@ -2403,7 +2407,7 @@ public class LocationProviderManager extends
         Preconditions.checkArgument(userId >= 0);
 
         boolean enabled = mState == STATE_STARTED
-                && mProvider.isAllowed()
+                && mProvider.getState().allowed
                 && mSettingsHelper.isLocationEnabled(userId);
 
         int index = mEnabled.indexOfKey(userId);
diff --git a/services/core/java/com/android/server/location/provider/MockableLocationProvider.java b/services/core/java/com/android/server/location/provider/MockableLocationProvider.java
index 021e8dbdb44e..22295fe7ba28 100644
--- a/services/core/java/com/android/server/location/provider/MockableLocationProvider.java
+++ b/services/core/java/com/android/server/location/provider/MockableLocationProvider.java
@@ -21,9 +21,7 @@ import static com.android.internal.util.ConcurrentUtils.DIRECT_EXECUTOR;
 import android.annotation.Nullable;
 import android.location.Location;
 import android.location.LocationResult;
-import android.location.provider.ProviderProperties;
 import android.location.provider.ProviderRequest;
-import android.location.util.identity.CallerIdentity;
 import android.os.Bundle;
 
 import com.android.internal.annotations.GuardedBy;
@@ -32,7 +30,6 @@ import com.android.internal.util.Preconditions;
 import java.io.FileDescriptor;
 import java.io.PrintWriter;
 import java.util.Collections;
-import java.util.Set;
 
 /**
  * Represents a location provider that may switch between a mock implementation and a real
@@ -290,21 +287,21 @@ public class MockableLocationProvider extends AbstractLocationProvider {
         Preconditions.checkState(!Thread.holdsLock(mOwnerLock));
 
         AbstractLocationProvider provider;
+        State providerState;
         synchronized (mOwnerLock) {
             provider = mProvider;
-            pw.println("allowed=" + isAllowed());
-            CallerIdentity identity = getIdentity();
-            if (identity != null) {
-                pw.println("identity=" + identity);
-            }
-            Set<String> extraAttributionTags = getExtraAttributionTags();
-            if (!extraAttributionTags.isEmpty()) {
-                pw.println("extra attribution tags=" + extraAttributionTags);
-            }
-            ProviderProperties properties = getProperties();
-            if (properties != null) {
-                pw.println("properties=" + properties);
-            }
+            providerState = getState();
+        }
+
+        pw.println("allowed=" + providerState.allowed);
+        if (providerState.identity != null) {
+            pw.println("identity=" + providerState.identity);
+        }
+        if (!providerState.extraAttributionTags.isEmpty()) {
+            pw.println("extra attribution tags=" + providerState.extraAttributionTags);
+        }
+        if (providerState.properties != null) {
+            pw.println("properties=" + providerState.properties);
         }
 
         if (provider != null) {
diff --git a/services/core/java/com/android/server/policy/AppOpsPolicy.java b/services/core/java/com/android/server/policy/AppOpsPolicy.java
index 94005b2c8361..22c370ef4dbe 100644
--- a/services/core/java/com/android/server/policy/AppOpsPolicy.java
+++ b/services/core/java/com/android/server/policy/AppOpsPolicy.java
@@ -33,14 +33,15 @@ import android.content.pm.ResolveInfo;
 import android.location.LocationManagerInternal;
 import android.net.Uri;
 import android.os.IBinder;
+import android.os.PackageTagsList;
 import android.os.Process;
 import android.os.UserHandle;
 import android.service.voice.VoiceInteractionManagerInternal;
 import android.service.voice.VoiceInteractionManagerInternal.HotwordDetectionServiceIdentity;
 import android.text.TextUtils;
-import android.util.ArrayMap;
 import android.util.ArraySet;
 import android.util.Log;
+import android.util.SparseArray;
 
 import com.android.internal.annotations.GuardedBy;
 import com.android.internal.util.function.DecFunction;
@@ -52,9 +53,9 @@ import com.android.internal.util.function.TriFunction;
 import com.android.internal.util.function.UndecFunction;
 import com.android.server.LocalServices;
 
+import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
 /**
@@ -67,11 +68,10 @@ public final class AppOpsPolicy implements AppOpsManagerInternal.CheckOpsDelegat
             "android:activity_recognition_allow_listed_tags";
     private static final String ACTIVITY_RECOGNITION_TAGS_SEPARATOR = ";";
 
-    private static ArraySet<String> sExpectedTags = new ArraySet<>(new String[] {
+    private static final ArraySet<String> sExpectedTags = new ArraySet<>(new String[] {
             "awareness_provider", "activity_recognition_provider", "network_location_provider",
             "network_location_calibration", "fused_location_provider", "geofencer_provider"});
 
-
     @NonNull
     private final Object mLock = new Object();
 
@@ -96,13 +96,22 @@ public final class AppOpsPolicy implements AppOpsManagerInternal.CheckOpsDelegat
      */
     @GuardedBy("mLock - writes only - see above")
     @NonNull
-    private final ConcurrentHashMap<Integer, ArrayMap<String, ArraySet<String>>> mLocationTags =
+    private final ConcurrentHashMap<Integer, PackageTagsList> mLocationTags =
             new ConcurrentHashMap<>();
 
+    // location tags can vary per uid - but we merge all tags under an app id into the final data
+    // structure above
+    @GuardedBy("mLock")
+    private final SparseArray<PackageTagsList> mPerUidLocationTags = new SparseArray<>();
+
+    // activity recognition currently only grabs tags from the APK manifest. we know that the
+    // manifest is the same for all users, so there's no need to track variations in tags across
+    // different users. if that logic ever changes, this might need to behave more like location
+    // tags above.
     @GuardedBy("mLock - writes only - see above")
     @NonNull
-    private final ConcurrentHashMap<Integer, ArrayMap<String, ArraySet<String>>>
-            mActivityRecognitionTags = new ConcurrentHashMap<>();
+    private final ConcurrentHashMap<Integer, PackageTagsList> mActivityRecognitionTags =
+            new ConcurrentHashMap<>();
 
     public AppOpsPolicy(@NonNull Context context) {
         mContext = context;
@@ -112,13 +121,28 @@ public final class AppOpsPolicy implements AppOpsManagerInternal.CheckOpsDelegat
 
         final LocationManagerInternal locationManagerInternal = LocalServices.getService(
                 LocationManagerInternal.class);
-        locationManagerInternal.setOnProviderLocationTagsChangeListener((providerTagInfo) -> {
-            synchronized (mLock) {
-                updateAllowListedTagsForPackageLocked(providerTagInfo.getUid(),
-                        providerTagInfo.getPackageName(), providerTagInfo.getTags(),
-                        mLocationTags);
-            }
-        });
+        locationManagerInternal.setLocationPackageTagsListener(
+                (uid, packageTagsList) -> {
+                    synchronized (mLock) {
+                        if (packageTagsList.isEmpty()) {
+                            mPerUidLocationTags.remove(uid);
+                        } else {
+                            mPerUidLocationTags.set(uid, packageTagsList);
+                        }
+
+                        int appId = UserHandle.getAppId(uid);
+                        PackageTagsList.Builder appIdTags = new PackageTagsList.Builder(1);
+                        int size = mPerUidLocationTags.size();
+                        for (int i = 0; i < size; i++) {
+                            if (UserHandle.getAppId(mPerUidLocationTags.keyAt(i)) == appId) {
+                                appIdTags.add(mPerUidLocationTags.valueAt(i));
+                            }
+                        }
+
+                        updateAllowListedTagsForPackageLocked(appId, appIdTags.build(),
+                                mLocationTags);
+                    }
+                });
 
         final IntentFilter intentFilter = new IntentFilter();
         intentFilter.addAction(Intent.ACTION_PACKAGE_ADDED);
@@ -306,95 +330,30 @@ public final class AppOpsPolicy implements AppOpsManagerInternal.CheckOpsDelegat
         }
         final String tagsList = resolvedService.serviceInfo.metaData.getString(
                 ACTIVITY_RECOGNITION_TAGS);
-        if (tagsList != null) {
-            final String[] tags = tagsList.split(ACTIVITY_RECOGNITION_TAGS_SEPARATOR);
+        if (!TextUtils.isEmpty(tagsList)) {
+            PackageTagsList packageTagsList = new PackageTagsList.Builder(1).add(
+                    resolvedService.serviceInfo.packageName,
+                    Arrays.asList(tagsList.split(ACTIVITY_RECOGNITION_TAGS_SEPARATOR))).build();
             synchronized (mLock) {
                 updateAllowListedTagsForPackageLocked(
-                        resolvedService.serviceInfo.applicationInfo.uid,
-                        resolvedService.serviceInfo.packageName, new ArraySet<>(tags),
+                        UserHandle.getAppId(resolvedService.serviceInfo.applicationInfo.uid),
+                        packageTagsList,
                         mActivityRecognitionTags);
             }
         }
     }
 
-    private static void updateAllowListedTagsForPackageLocked(int uid, String packageName,
-            Set<String> allowListedTags, ConcurrentHashMap<Integer, ArrayMap<String,
-            ArraySet<String>>> datastore) {
-        final int appId = UserHandle.getAppId(uid);
-        // We make a copy of the per UID state to limit our mutation to one
-        // operation in the underlying concurrent data structure.
-        ArrayMap<String, ArraySet<String>> appIdTags = datastore.get(appId);
-        if (appIdTags != null) {
-            appIdTags = new ArrayMap<>(appIdTags);
-        }
-
-        ArraySet<String> packageTags = (appIdTags != null) ? appIdTags.get(packageName) : null;
-        if (packageTags != null) {
-            packageTags = new ArraySet<>(packageTags);
-        }
-
-        if (allowListedTags != null && !allowListedTags.isEmpty()) {
-            if (packageTags != null) {
-                packageTags.clear();
-                packageTags.addAll(allowListedTags);
-            } else {
-                packageTags = new ArraySet<>(allowListedTags);
-            }
-            if (appIdTags == null) {
-                appIdTags = new ArrayMap<>();
-            }
-
-            // Remove any invalid tags
-            boolean nullRemoved = packageTags.remove(null);
-            boolean nullStrRemoved = packageTags.remove("null");
-            boolean emptyRemoved = packageTags.remove("");
-            if (nullRemoved || nullStrRemoved || emptyRemoved) {
-                Log.e(LOG_TAG, "Attempted to add invalid source attribution tag, removed "
-                        + "null: " + nullRemoved + " removed \"null\": " + nullStrRemoved
-                        + " removed empty string: " + emptyRemoved);
-            }
-
-            appIdTags.put(packageName, packageTags);
-            datastore.put(appId, appIdTags);
-        } else if (appIdTags != null) {
-            appIdTags.remove(packageName);
-            if (!appIdTags.isEmpty()) {
-                datastore.put(appId, appIdTags);
-            } else {
-                datastore.remove(appId);
-            }
-        }
+    private static void updateAllowListedTagsForPackageLocked(int appId,
+            PackageTagsList packageTagsList,
+            ConcurrentHashMap<Integer, PackageTagsList> datastore) {
+        datastore.put(appId, packageTagsList);
     }
 
     private static boolean isDatasourceAttributionTag(int uid, @NonNull String packageName,
-            @NonNull String attributionTag, @NonNull Map<Integer, ArrayMap<String,
-            ArraySet<String>>> mappedOps) {
+            @NonNull String attributionTag, @NonNull Map<Integer, PackageTagsList> mappedOps) {
         // Only a single lookup from the underlying concurrent data structure
-        final int appId = UserHandle.getAppId(uid);
-        final ArrayMap<String, ArraySet<String>> appIdTags = mappedOps.get(appId);
-        if (appIdTags != null) {
-            final ArraySet<String> packageTags = appIdTags.get(packageName);
-            if (packageTags != null && packageTags.contains(attributionTag)) {
-                if (packageName.equals("com.google.android.gms")
-                        && !sExpectedTags.contains(attributionTag)) {
-                    Log.i("AppOpsDebugRemapping", packageName + " tag "
-                            + attributionTag + " in " + packageTags);
-                }
-                return true;
-            }
-            if (packageName.equals("com.google.android.gms")
-                    && sExpectedTags.contains(attributionTag)) {
-                Log.i("AppOpsDebugRemapping", packageName + " tag " + attributionTag
-                        + " NOT in " + packageTags);
-            }
-        } else {
-            if (packageName.equals("com.google.android.gms")) {
-                Log.i("AppOpsDebugRemapping", "no package tags for uid " + uid
-                        + " package " + packageName);
-            }
-
-        }
-        return false;
+        final PackageTagsList appIdTags = mappedOps.get(UserHandle.getAppId(uid));
+        return appIdTags != null && appIdTags.contains(packageName, attributionTag);
     }
 
     private static int resolveLocationOp(int code) {
diff --git a/services/tests/mockingservicestests/src/com/android/server/location/provider/MockableLocationProviderTest.java b/services/tests/mockingservicestests/src/com/android/server/location/provider/MockableLocationProviderTest.java
index cf5db2e0db98..8532dbb7f38a 100644
--- a/services/tests/mockingservicestests/src/com/android/server/location/provider/MockableLocationProviderTest.java
+++ b/services/tests/mockingservicestests/src/com/android/server/location/provider/MockableLocationProviderTest.java
@@ -158,7 +158,7 @@ public class MockableLocationProviderTest {
 
     @Test
     public void testSetState() {
-        assertThat(mProvider.isAllowed()).isFalse();
+        assertThat(mProvider.getState().allowed).isFalse();
 
         AbstractLocationProvider.State newState;