Fix bugs in LocationManagerService and AppOpsPolicy

LocationManagerService was overwriting location source appops tag
information whenever a location provider changed state. Since multiple
location providers can correspond to the same uid/app id, this could
lead to loss of tags. This is fixed by recalculating the set of tag for
an entire app id any time a uid changes.

AppOpsPolicy had a bug where tag lists coming from different uids under
the same appid would inappropriately overwrite each other. This is fixed
by modifying the internal/external APIs to work in terms of appid rather
than uid, so that it is someone else's responsibility to ensure the
result is valid for the app id as a whole.

Also fixes a bug where LMS assumes tag listeners are added prior to any
providers. If listeners are added after some providers they will miss
updates. Fixed by always updating the listener on registration.

Bug: 190073375
Test: manual
Change-Id: Ib52d752d857b6ac3b6683186a8d1bbcb814fb798

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;