From 8f9038c0590cfb4d8a9ea36d8a898d64c4b73321 Mon Sep 17 00:00:00 2001
From: Robert Berry <robertberry@google.com>
Date: Mon, 26 Mar 2018 11:36:40 +0100
Subject: [PATCH] Fix setServerParams to not always set snapshot pending

Only updates should set snapshot pending. Setting the server params for
the first time should not set snapshot pending. If it did, then just
initializing the recovery agent would cause a snapshot to be made, even
if it contained no keys. Also, setting the snapshot to the same value as
it was previously should not set snapshot pending, for the exact same
reason. If the server params were to change, however, for some reason,
then a new snapshot must be made, so that it can be synced to the correct
vault.

Bug: 74949975
Test: runtest frameworks-services -p \
      com.android.server.locksettings.recoverablekeystore

Change-Id: Ie09284553f922de869be7bcd577d0f0eb9d0bbd3
---
 .../RecoverableKeyStoreManager.java           | 27 +++++++++--
 .../RecoverableKeyStoreManagerTest.java       | 45 +++++++++++++++++++
 2 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java b/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java
index 00abb37855f7b..f0bcb815ebe21 100644
--- a/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java
+++ b/services/core/java/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManager.java
@@ -317,14 +317,35 @@ public class RecoverableKeyStoreManager {
         mListenersStorage.setSnapshotListener(uid, intent);
     }
 
+    /**
+     * Set the server params for the user's key chain. This is used to uniquely identify a key
+     * chain. Along with the counter ID, it is used to uniquely identify an instance of a vault.
+     */
     public void setServerParams(@NonNull byte[] serverParams) throws RemoteException {
         checkRecoverKeyStorePermission();
         int userId = UserHandle.getCallingUserId();
         int uid = Binder.getCallingUid();
-        long updatedRows = mDatabase.setServerParams(userId, uid, serverParams);
-        if (updatedRows > 0) {
-            mDatabase.setShouldCreateSnapshot(userId, uid, true);
+
+        byte[] currentServerParams = mDatabase.getServerParams(userId, uid);
+
+        if (Arrays.equals(serverParams, currentServerParams)) {
+            Log.v(TAG, "Not updating server params - same as old value.");
+            return;
         }
+
+        long updatedRows = mDatabase.setServerParams(userId, uid, serverParams);
+        if (updatedRows < 1) {
+            throw new ServiceSpecificException(
+                    ERROR_SERVICE_INTERNAL_ERROR, "Database failure trying to set server params.");
+        }
+
+        if (currentServerParams == null) {
+            Log.i(TAG, "Initialized server params.");
+            return;
+        }
+
+        Log.i(TAG, "Updated server params. Snapshot pending.");
+        mDatabase.setShouldCreateSnapshot(userId, uid, true);
     }
 
     /**
diff --git a/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManagerTest.java b/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManagerTest.java
index fc2da39da7eb2..445e50a4f8198 100644
--- a/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManagerTest.java
+++ b/services/tests/servicestests/src/com/android/server/locksettings/recoverablekeystore/RecoverableKeyStoreManagerTest.java
@@ -895,6 +895,51 @@ public class RecoverableKeyStoreManagerTest {
         verify(mMockListenersStorage).setSnapshotListener(eq(uid), any(PendingIntent.class));
     }
 
+    @Test
+    public void setServerParams_updatesServerParams() throws Exception {
+        int uid = Binder.getCallingUid();
+        int userId = UserHandle.getCallingUserId();
+        byte[] serverParams = new byte[] { 1 };
+
+        mRecoverableKeyStoreManager.setServerParams(serverParams);
+
+        assertThat(mRecoverableKeyStoreDb.getServerParams(userId, uid)).isEqualTo(serverParams);
+    }
+
+    @Test
+    public void setServerParams_doesNotSetSnapshotPendingIfInitializing() throws Exception {
+        int uid = Binder.getCallingUid();
+        int userId = UserHandle.getCallingUserId();
+        byte[] serverParams = new byte[] { 1 };
+
+        mRecoverableKeyStoreManager.setServerParams(serverParams);
+
+        assertThat(mRecoverableKeyStoreDb.getShouldCreateSnapshot(userId, uid)).isFalse();
+    }
+
+    @Test
+    public void setServerParams_doesNotSetSnapshotPendingIfSettingSameValue() throws Exception {
+        int uid = Binder.getCallingUid();
+        int userId = UserHandle.getCallingUserId();
+        byte[] serverParams = new byte[] { 1 };
+
+        mRecoverableKeyStoreManager.setServerParams(serverParams);
+        mRecoverableKeyStoreManager.setServerParams(serverParams);
+
+        assertThat(mRecoverableKeyStoreDb.getShouldCreateSnapshot(userId, uid)).isFalse();
+    }
+
+    @Test
+    public void setServerParams_setsSnapshotPendingIfUpdatingValue() throws Exception {
+        int uid = Binder.getCallingUid();
+        int userId = UserHandle.getCallingUserId();
+
+        mRecoverableKeyStoreManager.setServerParams(new byte[] { 1 });
+        mRecoverableKeyStoreManager.setServerParams(new byte[] { 2 });
+
+        assertThat(mRecoverableKeyStoreDb.getShouldCreateSnapshot(userId, uid)).isTrue();
+    }
+
     @Test
     public void setRecoverySecretTypes() throws Exception {
         int[] types1 = new int[]{11, 2000};