Merge "Add proper permission check and multi-user handling to GameManagerService" into sc-dev

core/java/android/app/GameManager.java

@@ -16,7 +16,9 @@
 
 package android.app;
 
+import android.Manifest;
 import android.annotation.IntDef;
+import android.annotation.RequiresPermission;
 import android.annotation.SystemService;
 import android.annotation.UserHandleAware;
 import android.content.Context;
@@ -73,8 +75,8 @@ public final class GameManager {
     /**
      * Returns the game mode for the given package.
      */
-    // TODO(b/178111358): Add @RequiresPermission.
     @UserHandleAware
+    @RequiresPermission(Manifest.permission.MANAGE_GAME_MODE)
     public @GameMode int getGameMode(String packageName) {
         try {
             return mService.getGameMode(packageName, mContext.getUserId());
@@ -86,8 +88,8 @@ public final class GameManager {
     /**
      * Sets the game mode for the given package.
      */
-    // TODO(b/178111358): Add @RequiresPermission.
     @UserHandleAware
+    @RequiresPermission(Manifest.permission.MANAGE_GAME_MODE)
     public void setGameMode(String packageName, @GameMode int gameMode) {
         try {
             mService.setGameMode(packageName, gameMode, mContext.getUserId());

core/res/AndroidManifest.xml

@@ -5447,6 +5447,11 @@
     <permission android:name="android.permission.MANAGE_TOAST_RATE_LIMITING"
                 android:protectionLevel="signature" />
 
+    <!-- Allows managing the Game Mode
+     @hide Used internally. -->
+    <permission android:name="android.permission.MANAGE_GAME_MODE"
+                android:protectionLevel="signature" />
+
     <!-- Attribution for Geofencing service. -->
     <attribution android:tag="GeofencingService" android:label="@string/geofencing_service"/>
     <!-- Attribution for Country Detector. -->

services/core/java/com/android/server/app/GameManagerService.java

@@ -16,17 +16,23 @@
 
 package com.android.server.app;
 
+import android.Manifest;
 import android.annotation.NonNull;
+import android.annotation.RequiresPermission;
+import android.app.ActivityManager;
 import android.app.GameManager;
 import android.app.GameManager.GameMode;
 import android.app.IGameManagerService;
 import android.content.Context;
+import android.content.pm.PackageManager;
+import android.os.Binder;
 import android.os.Environment;
 import android.os.Handler;
 import android.os.Looper;
 import android.os.Message;
 import android.os.Process;
 import android.util.ArrayMap;
+import android.util.Log;
 import android.util.Slog;
 
 import com.android.internal.annotations.GuardedBy;
@@ -81,6 +87,14 @@ public final class GameManagerService extends IGameManagerService.Stub {
             switch (msg.what) {
                 case WRITE_SETTINGS: {
                     final int userId = (int) msg.obj;
+                    if (userId < 0) {
+                        Slog.wtf(TAG, "Attempt to write settings for invalid user: " + userId);
+                        synchronized (mLock) {
+                            removeMessages(WRITE_SETTINGS, msg.obj);
+                        }
+                        break;
+                    }
+
                     Process.setThreadPriority(Process.THREAD_PRIORITY_DEFAULT);
                     synchronized (mLock) {
                         removeMessages(WRITE_SETTINGS, msg.obj);
@@ -94,6 +108,15 @@ public final class GameManagerService extends IGameManagerService.Stub {
                 }
                 case REMOVE_SETTINGS: {
                     final int userId = (int) msg.obj;
+                    if (userId < 0) {
+                        Slog.wtf(TAG, "Attempt to write settings for invalid user: " + userId);
+                        synchronized (mLock) {
+                            removeMessages(WRITE_SETTINGS, msg.obj);
+                            removeMessages(REMOVE_SETTINGS, msg.obj);
+                        }
+                        break;
+                    }
+
                     synchronized (mLock) {
                         // Since the user was removed, ignore previous write message
                         // and do write here.
@@ -146,9 +169,23 @@ public final class GameManagerService extends IGameManagerService.Stub {
         }
     }
 
-    //TODO(b/178111358) Add proper permission check and multi-user handling
+    private boolean hasPermission(String permission) {
+        return mContext.checkCallingOrSelfPermission(permission)
+                == PackageManager.PERMISSION_GRANTED;
+    }
+
     @Override
+    @RequiresPermission(Manifest.permission.MANAGE_GAME_MODE)
     public @GameMode int getGameMode(String packageName, int userId) {
+        if (!hasPermission(Manifest.permission.MANAGE_GAME_MODE)) {
+            Log.w(TAG, String.format("Caller or self does not have permission.MANAGE_GAME_MODE"));
+            return GameManager.GAME_MODE_UNSUPPORTED;
+        }
+
+        userId = ActivityManager.handleIncomingUser(Binder.getCallingPid(),
+                Binder.getCallingUid(), userId, false, true, "getGameMode",
+                "com.android.server.app.GameManagerService");
+
         synchronized (mLock) {
             if (!mSettings.containsKey(userId)) {
                 return GameManager.GAME_MODE_UNSUPPORTED;
@@ -158,9 +195,18 @@ public final class GameManagerService extends IGameManagerService.Stub {
         }
     }
 
-    //TODO(b/178111358) Add proper permission check and multi-user handling
     @Override
+    @RequiresPermission(Manifest.permission.MANAGE_GAME_MODE)
     public void setGameMode(String packageName, @GameMode int gameMode, int userId) {
+        if (!hasPermission(Manifest.permission.MANAGE_GAME_MODE)) {
+            Log.w(TAG, String.format("Caller or self does not have permission.MANAGE_GAME_MODE"));
+            return;
+        }
+
+        userId = ActivityManager.handleIncomingUser(Binder.getCallingPid(),
+                Binder.getCallingUid(), userId, false, true, "setGameMode",
+                "com.android.server.app.GameManagerService");
+
         synchronized (mLock) {
             if (!mSettings.containsKey(userId)) {
                 return;

services/tests/servicestests/src/com/android/server/app/GameManagerServiceTests.java

@@ -18,14 +18,23 @@ package com.android.server.app;
 
 import static org.junit.Assert.assertEquals;
 
+import android.Manifest;
 import android.app.GameManager;
+import android.content.Context;
+import android.content.ContextWrapper;
+import android.content.pm.PackageManager;
 
 import androidx.test.InstrumentationRegistry;
 import androidx.test.filters.SmallTest;
 import androidx.test.runner.AndroidJUnit4;
 
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Mock;
+
+import java.util.HashMap;
+import java.util.function.Supplier;
 
 @RunWith(AndroidJUnit4.class)
 @SmallTest
@@ -37,15 +46,88 @@ public class GameManagerServiceTests {
     private static final int USER_ID_1 = 1001;
     private static final int USER_ID_2 = 1002;
 
+    // Stolen from ConnectivityServiceTest.MockContext
+    class MockContext extends ContextWrapper {
+        private static final String TAG = "MockContext";
+
+        // Map of permission name -> PermissionManager.Permission_{GRANTED|DENIED} constant
+        private final HashMap<String, Integer> mMockedPermissions = new HashMap<>();
+
+        MockContext(Context base) {
+            super(base);
+        }
+
+        /**
+         * Mock checks for the specified permission, and have them behave as per {@code granted}.
+         *
+         * <p>Passing null reverts to default behavior, which does a real permission check on the
+         * test package.
+         * @param granted One of {@link PackageManager#PERMISSION_GRANTED} or
+         *                {@link PackageManager#PERMISSION_DENIED}.
+         */
+        public void setPermission(String permission, Integer granted) {
+            mMockedPermissions.put(permission, granted);
+        }
+
+        private int checkMockedPermission(String permission, Supplier<Integer> ifAbsent) {
+            final Integer granted = mMockedPermissions.get(permission);
+            return granted != null ? granted : ifAbsent.get();
+        }
+
+        @Override
+        public int checkPermission(String permission, int pid, int uid) {
+            return checkMockedPermission(
+                    permission, () -> super.checkPermission(permission, pid, uid));
+        }
+
+        @Override
+        public int checkCallingOrSelfPermission(String permission) {
+            return checkMockedPermission(
+                    permission, () -> super.checkCallingOrSelfPermission(permission));
+        }
+
+        @Override
+        public void enforceCallingOrSelfPermission(String permission, String message) {
+            final Integer granted = mMockedPermissions.get(permission);
+            if (granted == null) {
+                super.enforceCallingOrSelfPermission(permission, message);
+                return;
+            }
+
+            if (!granted.equals(PackageManager.PERMISSION_GRANTED)) {
+                throw new SecurityException("[Test] permission denied: " + permission);
+            }
+        }
+    }
+
+    @Mock
+    private MockContext mMockContext;
+
+    @Before
+    public void setUp() throws Exception {
+        mMockContext = new MockContext(InstrumentationRegistry.getContext());
+    }
+
+    private void mockModifyGameModeGranted() {
+        mMockContext.setPermission(Manifest.permission.MANAGE_GAME_MODE,
+                PackageManager.PERMISSION_GRANTED);
+    }
+
+    private void mockModifyGameModeDenied() {
+        mMockContext.setPermission(Manifest.permission.MANAGE_GAME_MODE,
+                PackageManager.PERMISSION_DENIED);
+    }
+
     /**
      * By default game mode is not supported.
      */
     @Test
     public void testGameModeDefaultValue() {
-        GameManagerService gameManagerService =
+        GameManagerService gameManagerService = new GameManagerService(mMockContext);
-                new GameManagerService(InstrumentationRegistry.getContext());
         gameManagerService.onUserStarting(USER_ID_1);
 
+        mockModifyGameModeGranted();
+
         assertEquals(GameManager.GAME_MODE_UNSUPPORTED,
                 gameManagerService.getGameMode(PACKAGE_NAME_0, USER_ID_1));
     }
@@ -55,10 +137,11 @@ public class GameManagerServiceTests {
      */
     @Test
     public void testDefaultValueForNonexistentUser() {
-        GameManagerService gameManagerService =
+        GameManagerService gameManagerService = new GameManagerService(mMockContext);
-                new GameManagerService(InstrumentationRegistry.getContext());
         gameManagerService.onUserStarting(USER_ID_1);
 
+        mockModifyGameModeGranted();
+
         gameManagerService.setGameMode(PACKAGE_NAME_1, GameManager.GAME_MODE_STANDARD, USER_ID_2);
         assertEquals(GameManager.GAME_MODE_UNSUPPORTED,
                 gameManagerService.getGameMode(PACKAGE_NAME_1, USER_ID_2));
@@ -69,10 +152,11 @@ public class GameManagerServiceTests {
      */
     @Test
     public void testGameMode() {
-        GameManagerService gameManagerService =
+        GameManagerService gameManagerService =  new GameManagerService(mMockContext);
-                new GameManagerService(InstrumentationRegistry.getContext());
         gameManagerService.onUserStarting(USER_ID_1);
 
+        mockModifyGameModeGranted();
+
         assertEquals(GameManager.GAME_MODE_UNSUPPORTED,
                 gameManagerService.getGameMode(PACKAGE_NAME_1, USER_ID_1));
         gameManagerService.setGameMode(PACKAGE_NAME_1, GameManager.GAME_MODE_STANDARD, USER_ID_1);
@@ -83,4 +167,48 @@ public class GameManagerServiceTests {
         assertEquals(GameManager.GAME_MODE_PERFORMANCE,
                 gameManagerService.getGameMode(PACKAGE_NAME_1, USER_ID_1));
     }
+
+    /**
+     * Test permission.MANAGE_GAME_MODE is checked
+     */
+    @Test
+    public void testGetGameModePermissionDenied() {
+        GameManagerService gameManagerService =  new GameManagerService(mMockContext);
+        gameManagerService.onUserStarting(USER_ID_1);
+
+        // Update the game mode so we can read back something valid.
+        mockModifyGameModeGranted();
+        gameManagerService.setGameMode(PACKAGE_NAME_1, GameManager.GAME_MODE_STANDARD, USER_ID_1);
+        assertEquals(GameManager.GAME_MODE_STANDARD,
+                gameManagerService.getGameMode(PACKAGE_NAME_1, USER_ID_1));
+
+        // Deny permission.MANAGE_GAME_MODE and verify we get back GameManager.GAME_MODE_UNSUPPORTED
+        mockModifyGameModeDenied();
+        assertEquals(GameManager.GAME_MODE_UNSUPPORTED,
+                gameManagerService.getGameMode(PACKAGE_NAME_1, USER_ID_1));
+    }
+
+    /**
+     * Test permission.MANAGE_GAME_MODE is checked
+     */
+    @Test
+    public void testSetGameModePermissionDenied() {
+        GameManagerService gameManagerService =  new GameManagerService(mMockContext);
+        gameManagerService.onUserStarting(USER_ID_1);
+
+        // Update the game mode so we can read back something valid.
+        mockModifyGameModeGranted();
+        gameManagerService.setGameMode(PACKAGE_NAME_1, GameManager.GAME_MODE_STANDARD, USER_ID_1);
+        assertEquals(GameManager.GAME_MODE_STANDARD,
+                gameManagerService.getGameMode(PACKAGE_NAME_1, USER_ID_1));
+
+        // Deny permission.MANAGE_GAME_MODE and verify the game mode is not updated.
+        mockModifyGameModeDenied();
+        gameManagerService.setGameMode(PACKAGE_NAME_1, GameManager.GAME_MODE_PERFORMANCE,
+                USER_ID_1);
+
+        mockModifyGameModeGranted();
+        assertEquals(GameManager.GAME_MODE_STANDARD,
+                gameManagerService.getGameMode(PACKAGE_NAME_1, USER_ID_1));
+    }
 }