Revoke Uri access after a NotificationListener is removed. am: b28236132d

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/12434415

Change-Id: I459c36327adf8628159b60e63ef35b3a1ade936d

5 files changed, 116 insertions, 47 deletions

diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java
index ee1a4b28141a..13afaf856ba0 100755
--- a/services/core/java/com/android/server/notification/NotificationManagerService.java
+++ b/services/core/java/com/android/server/notification/NotificationManagerService.java
@@ -7737,6 +7737,13 @@ public class NotificationManagerService extends SystemService {
     @VisibleForTesting
     void updateUriPermissions(@Nullable NotificationRecord newRecord,
             @Nullable NotificationRecord oldRecord, String targetPkg, int targetUserId) {
+        updateUriPermissions(newRecord, oldRecord, targetPkg, targetUserId, false);
+    }
+
+    @VisibleForTesting
+    void updateUriPermissions(@Nullable NotificationRecord newRecord,
+            @Nullable NotificationRecord oldRecord, String targetPkg, int targetUserId,
+            boolean onlyRevokeCurrentTarget) {
         final String key = (newRecord != null) ? newRecord.getKey() : oldRecord.getKey();
         if (DBG) Slog.d(TAG, key + ": updating permissions");
 
@@ -7764,7 +7771,9 @@ public class NotificationManagerService extends SystemService {
         }
 
         // If we have no Uris to grant, but an existing owner, go destroy it
-        if (newUris == null && permissionOwner != null) {
+        // When revoking permissions of a single listener, destroying the owner will revoke
+        // permissions of other listeners who need to keep access.
+        if (newUris == null && permissionOwner != null && !onlyRevokeCurrentTarget) {
             destroyPermissionOwner(permissionOwner, UserHandle.getUserId(oldRecord.getUid()), key);
             permissionOwner = null;
         }
@@ -7787,9 +7796,20 @@ public class NotificationManagerService extends SystemService {
                 final Uri uri = oldUris.valueAt(i);
                 if (newUris == null || !newUris.contains(uri)) {
                     if (DBG) Slog.d(TAG, key + ": revoking " + uri);
-                    int userId = ContentProvider.getUserIdFromUri(
-                            uri, UserHandle.getUserId(oldRecord.getUid()));
-                    revokeUriPermission(permissionOwner, uri, userId);
+                    if (onlyRevokeCurrentTarget) {
+                        // We're revoking permission from one listener only; other listeners may
+                        // still need access because the notification may still exist
+                        revokeUriPermission(permissionOwner, uri,
+                                UserHandle.getUserId(oldRecord.getUid()), targetPkg, targetUserId);
+                    } else {
+                        // This is broad to unilaterally revoke permissions to this Uri as granted
+                        // by this notification.  But this code-path can only be used when the
+                        // reason for revoking is that the notification posted again without this
+                        // Uri, not when removing an individual listener.
+                        revokeUriPermission(permissionOwner, uri,
+                                UserHandle.getUserId(oldRecord.getUid()),
+                                null, UserHandle.USER_ALL);
+                    }
                 }
             }
         }
@@ -7818,8 +7838,10 @@ public class NotificationManagerService extends SystemService {
         }
     }
 
-    private void revokeUriPermission(IBinder owner, Uri uri, int userId) {
+    private void revokeUriPermission(IBinder owner, Uri uri, int sourceUserId, String targetPkg,
+            int targetUserId) {
         if (uri == null || !ContentResolver.SCHEME_CONTENT.equals(uri.getScheme())) return;
+        int userId = ContentProvider.getUserIdFromUri(uri, sourceUserId);
 
         final long ident = Binder.clearCallingIdentity();
         try {
@@ -7827,7 +7849,7 @@ public class NotificationManagerService extends SystemService {
                     owner,
                     ContentProvider.getUriWithoutUserId(uri),
                     Intent.FLAG_GRANT_READ_URI_PERMISSION,
-                    userId);
+                    userId, targetPkg, targetUserId);
         } finally {
             Binder.restoreCallingIdentity(ident);
         }
@@ -9195,7 +9217,7 @@ public class NotificationManagerService extends SystemService {
             final NotificationRankingUpdate update;
             synchronized (mNotificationLock) {
                 update = makeRankingUpdateLocked(info);
-                grantUriPermissionsForActiveNotificationsLocked(info);
+                updateUriPermissionsForActiveNotificationsLocked(info, true);
             }
             try {
                 listener.onListenerConnected(update);
@@ -9207,6 +9229,7 @@ public class NotificationManagerService extends SystemService {
         @Override
         @GuardedBy("mNotificationLock")
         protected void onServiceRemovedLocked(ManagedServiceInfo removed) {
+            updateUriPermissionsForActiveNotificationsLocked(removed, false);
             if (removeDisabledHints(removed)) {
                 updateListenerHintsLocked();
                 updateEffectsSuppressorLocked();
@@ -9273,8 +9296,7 @@ public class NotificationManagerService extends SystemService {
 
                 for (final ManagedServiceInfo info : getServices()) {
                     boolean sbnVisible = isVisibleToListener(sbn, info);
-                    boolean oldSbnVisible = oldSbn != null ? isVisibleToListener(oldSbn, info)
-                            : false;
+                    boolean oldSbnVisible = (oldSbn != null) && isVisibleToListener(oldSbn, info);
                     // This notification hasn't been and still isn't visible -> ignore.
                     if (!oldSbnVisible && !sbnVisible) {
                         continue;
@@ -9317,28 +9339,42 @@ public class NotificationManagerService extends SystemService {
         }
 
         /**
-         * Synchronously grant permissions to Uris for all active and visible notifications to the
-         * NotificationListenerService provided.
+         * Synchronously grant or revoke permissions to Uris for all active and visible
+         * notifications to just the NotificationListenerService provided.
          */
         @GuardedBy("mNotificationLock")
-        private void grantUriPermissionsForActiveNotificationsLocked(ManagedServiceInfo info) {
+        private void updateUriPermissionsForActiveNotificationsLocked(
+                ManagedServiceInfo info, boolean grant) {
             try {
                 for (final NotificationRecord r : mNotificationList) {
-                    // This notification isn't visible -> ignore.
-                    if (!isVisibleToListener(r.getSbn(), info)) {
+                    // When granting permissions, ignore notifications which are invisible.
+                    // When revoking permissions, all notifications are invisible, so process all.
+                    if (grant && !isVisibleToListener(r.getSbn(), info)) {
                         continue;
                     }
                     // If the notification is hidden, permissions are not required by the listener.
                     if (r.isHidden() && info.targetSdkVersion < Build.VERSION_CODES.P) {
                         continue;
                     }
-                    // Grant access before listener is initialized
+                    // Grant or revoke access synchronously
                     final int targetUserId = (info.userid == UserHandle.USER_ALL)
                             ? UserHandle.USER_SYSTEM : info.userid;
-                    updateUriPermissions(r, null, info.component.getPackageName(), targetUserId);
+                    if (grant) {
+                        // Grant permissions by passing arguments as if the notification is new.
+                        updateUriPermissions(/* newRecord */ r, /* oldRecord */ null,
+                                info.component.getPackageName(), targetUserId);
+                    } else {
+                        // Revoke permissions by passing arguments as if the notification was
+                        // removed, but set `onlyRevokeCurrentTarget` to avoid revoking permissions
+                        // granted to *other* targets by this notification's URIs.
+                        updateUriPermissions(/* newRecord */ null, /* oldRecord */ r,
+                                info.component.getPackageName(), targetUserId,
+                                /* onlyRevokeCurrentTarget */ true);
+                    }
                 }
             } catch (Exception e) {
-                Slog.e(TAG, "Could not grant Uri permissions to " + info.component, e);
+                Slog.e(TAG, "Could not " + (grant ? "grant" : "revoke") + " Uri permissions to "
+                        + info.component, e);
             }
         }
 
@@ -9377,18 +9413,11 @@ public class NotificationManagerService extends SystemService {
                 final NotificationStats stats = mAssistants.isServiceTokenValidLocked(info.service)
                         ? notificationStats : null;
                 final NotificationRankingUpdate update = makeRankingUpdateLocked(info);
-                mHandler.post(new Runnable() {
-                    @Override
-                    public void run() {
-                        notifyRemoved(info, sbnLight, update, stats, reason);
-                    }
-                });
+                mHandler.post(() -> notifyRemoved(info, sbnLight, update, stats, reason));
             }
 
             // Revoke access after all listeners have been updated
-            mHandler.post(() -> {
-                updateUriPermissions(null, r, null, UserHandle.USER_SYSTEM);
-            });
+            mHandler.post(() -> updateUriPermissions(null, r, null, UserHandle.USER_SYSTEM));
         }
 
         /**
diff --git a/services/core/java/com/android/server/uri/UriGrantsManagerInternal.java b/services/core/java/com/android/server/uri/UriGrantsManagerInternal.java
index cdb61995c336..5772dea287fc 100644
--- a/services/core/java/com/android/server/uri/UriGrantsManagerInternal.java
+++ b/services/core/java/com/android/server/uri/UriGrantsManagerInternal.java
@@ -75,10 +75,31 @@ public interface UriGrantsManagerInternal {
     void removeUriPermissionsForPackage(
             String packageName, int userHandle, boolean persistable, boolean targetOnly);
     /**
-     * @param uri This uri must NOT contain an embedded userId.
+     * Remove any {@link UriPermission} associated with the owner whose values match the given
+     * filtering parameters.
+     *
+     * @param token An opaque owner token as returned by {@link #newUriPermissionOwner(String)}.
+     * @param uri This uri must NOT contain an embedded userId. {@code null} to apply to all Uris.
+     * @param mode The modes (as a bitmask) to revoke.
      * @param userId The userId in which the uri is to be resolved.
      */
     void revokeUriPermissionFromOwner(IBinder token, Uri uri, int mode, int userId);
+
+    /**
+     * Remove any {@link UriPermission} associated with the owner whose values match the given
+     * filtering parameters.
+     *
+     * @param token An opaque owner token as returned by {@link #newUriPermissionOwner(String)}.
+     * @param uri This uri must NOT contain an embedded userId. {@code null} to apply to all Uris.
+     * @param mode The modes (as a bitmask) to revoke.
+     * @param userId The userId in which the uri is to be resolved.
+     * @param targetPkg Calling package name to match, or {@code null} to apply to all packages.
+     * @param targetUserId Calling user to match, or {@link UserHandle#USER_ALL} to apply to all
+     *                     users.
+     */
+    void revokeUriPermissionFromOwner(IBinder token, Uri uri, int mode, int userId,
+            String targetPkg, int targetUserId);
+
     boolean checkAuthorityGrants(
             int callingUid, ProviderInfo cpi, int userId, boolean checkUser);
     void dump(PrintWriter pw, boolean dumpAll, String dumpPackage);
diff --git a/services/core/java/com/android/server/uri/UriGrantsManagerService.java b/services/core/java/com/android/server/uri/UriGrantsManagerService.java
index f14c3a53940d..8eefd8fa4a91 100644
--- a/services/core/java/com/android/server/uri/UriGrantsManagerService.java
+++ b/services/core/java/com/android/server/uri/UriGrantsManagerService.java
@@ -51,7 +51,6 @@ import android.app.AppGlobals;
 import android.app.GrantedUriPermission;
 import android.app.IUriGrantsManager;
 import android.content.ClipData;
-import android.content.ComponentName;
 import android.content.ContentProvider;
 import android.content.ContentResolver;
 import android.content.Context;
@@ -88,11 +87,11 @@ import com.android.server.LocalServices;
 import com.android.server.SystemService;
 import com.android.server.SystemServiceManager;
 
-import libcore.io.IoUtils;
-
 import com.google.android.collect.Lists;
 import com.google.android.collect.Maps;
 
+import libcore.io.IoUtils;
+
 import org.xmlpull.v1.XmlPullParser;
 import org.xmlpull.v1.XmlPullParserException;
 import org.xmlpull.v1.XmlSerializer;
@@ -1431,16 +1430,18 @@ public class UriGrantsManagerService extends IUriGrantsManager.Stub {
 
         @Override
         public void revokeUriPermissionFromOwner(IBinder token, Uri uri, int mode, int userId) {
+            revokeUriPermissionFromOwner(token, uri, mode, userId, null, UserHandle.USER_ALL);
+        }
+
+        @Override
+        public void revokeUriPermissionFromOwner(IBinder token, Uri uri, int mode, int userId,
+                String targetPkg, int targetUserId) {
             final UriPermissionOwner owner = UriPermissionOwner.fromExternalToken(token);
             if (owner == null) {
                 throw new IllegalArgumentException("Unknown owner: " + token);
             }
-
-            if (uri == null) {
-                owner.removeUriPermissions(mode);
-            } else {
-                owner.removeUriPermission(new GrantUri(userId, uri, mode), mode);
-            }
+            GrantUri grantUri = uri == null ? null : new GrantUri(userId, uri, mode);
+            owner.removeUriPermission(grantUri, mode, targetPkg, targetUserId);
         }
 
         @Override
diff --git a/services/core/java/com/android/server/uri/UriPermissionOwner.java b/services/core/java/com/android/server/uri/UriPermissionOwner.java
index 2b404a43a338..0c263997a8b5 100644
--- a/services/core/java/com/android/server/uri/UriPermissionOwner.java
+++ b/services/core/java/com/android/server/uri/UriPermissionOwner.java
@@ -21,6 +21,7 @@ import static android.content.Intent.FLAG_GRANT_WRITE_URI_PERMISSION;
 
 import android.os.Binder;
 import android.os.IBinder;
+import android.os.UserHandle;
 import android.util.ArraySet;
 import android.util.proto.ProtoOutputStream;
 
@@ -74,30 +75,47 @@ public class UriPermissionOwner {
     }
 
     void removeUriPermission(GrantUri grantUri, int mode) {
+        removeUriPermission(grantUri, mode, null, UserHandle.USER_ALL);
+    }
+
+    void removeUriPermission(GrantUri grantUri, int mode, String targetPgk, int targetUserId) {
         if ((mode & FLAG_GRANT_READ_URI_PERMISSION) != 0 && mReadPerms != null) {
             Iterator<UriPermission> it = mReadPerms.iterator();
             while (it.hasNext()) {
                 UriPermission perm = it.next();
-                if (grantUri == null || grantUri.equals(perm.uri)) {
-                    perm.removeReadOwner(this);
-                    mService.removeUriPermissionIfNeeded(perm);
-                    it.remove();
+                if (grantUri != null && !grantUri.equals(perm.uri)) {
+                    continue;
+                }
+                if (targetPgk != null && !targetPgk.equals(perm.targetPkg)) {
+                    continue;
                 }
+                if (targetUserId != UserHandle.USER_ALL && targetUserId != perm.targetUserId) {
+                    continue;
+                }
+                perm.removeReadOwner(this);
+                mService.removeUriPermissionIfNeeded(perm);
+                it.remove();
             }
             if (mReadPerms.isEmpty()) {
                 mReadPerms = null;
             }
         }
-        if ((mode & FLAG_GRANT_WRITE_URI_PERMISSION) != 0
-                && mWritePerms != null) {
+        if ((mode & FLAG_GRANT_WRITE_URI_PERMISSION) != 0 && mWritePerms != null) {
             Iterator<UriPermission> it = mWritePerms.iterator();
             while (it.hasNext()) {
                 UriPermission perm = it.next();
-                if (grantUri == null || grantUri.equals(perm.uri)) {
-                    perm.removeWriteOwner(this);
-                    mService.removeUriPermissionIfNeeded(perm);
-                    it.remove();
+                if (grantUri != null && !grantUri.equals(perm.uri)) {
+                    continue;
+                }
+                if (targetPgk != null && !targetPgk.equals(perm.targetPkg)) {
+                    continue;
+                }
+                if (targetUserId != UserHandle.USER_ALL && targetUserId != perm.targetUserId) {
+                    continue;
                 }
+                perm.removeWriteOwner(this);
+                mService.removeUriPermissionIfNeeded(perm);
+                it.remove();
             }
             if (mWritePerms.isEmpty()) {
                 mWritePerms = null;
diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java
index 16aa87b3e59c..52e08187b2ca 100755
--- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java
+++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java
@@ -3865,7 +3865,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase {
         mService.updateUriPermissions(recordB, recordA, mContext.getPackageName(),
                 USER_SYSTEM);
         verify(mUgmInternal, times(1)).revokeUriPermissionFromOwner(any(),
-                eq(message1.getDataUri()), anyInt(), anyInt());
+                eq(message1.getDataUri()), anyInt(), anyInt(), eq(null), eq(-1));
 
         // Update back means we grant access to first again
         reset(mUgm);