Merge pull request #51 from billkalter/fix-internal-id-reindexing

Fixed errors grandfathering-in pre-existing API keys to internal IDs

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/apikey/ApiKey.java

@@ -4,7 +4,6 @@
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.hash.Hashing;
 
 import java.util.List;
 import java.util.Set;
@@ -23,20 +22,6 @@ public ApiKey(@JsonProperty("id") String key,
                   @JsonProperty("internalId") String internalId,
                   @JsonProperty("roles") List<String> roles) {
 
-        // API keys have been in use since before internal IDs were introduced.  To grandfather in those keys we'll
-        // use a hash of the API key as the internal ID.
-        this(key, resolveInternalId(key, internalId), ImmutableSet.copyOf(roles));
-    }
-
-    private static String resolveInternalId(String key, String internalId) {
-        if (internalId != null) {
-            return internalId;
-        }
-        // SHA-256 is a little heavy but it has two advantages:
-        // 1.  It is the same algorithm currently used to store API keys by the permission manager so there isn't a
-        //     potential conflict between keys.
-        // 2.  The API keys are cached by Shiro so this conversion will only take place when the key needs to
-        //     be (re)loaded.
-        return Hashing.sha256().hashUnencodedChars(key).toString();
+        this(key, internalId, ImmutableSet.copyOf(roles));
     }
 }

auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/TableAuthIdentityManager.java

@@ -17,6 +17,7 @@
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
 import com.google.common.hash.HashFunction;
 
 import javax.annotation.Nullable;
@@ -37,6 +38,7 @@
 public class TableAuthIdentityManager<T extends AuthIdentity> implements AuthIdentityManager<T> {
 
     private final static String ID = "id";
+    private final static String INTERNAL_ID = "internalId";
     private final static String MASKED_ID = "maskedId";
     private final static String HASHED_ID = "hashedId";
 
@@ -80,12 +82,23 @@ private T convertDataStoreEntryToIdentity(String id, Map<String, Object> map) {
             return null;
         }
 
+        // Make a copy of the map to avoid mutating the method parameter as a side-effect
+        Map<String, Object> identityMap = Maps.newHashMap(map);
+
+        // Identities have been in use since before internal IDs were introduced.  To grandfather in those keys we'll
+        // use the hash of the identity's ID as the internal ID.
+        if (!identityMap.containsKey(INTERNAL_ID)) {
+            identityMap.put(INTERNAL_ID, Intrinsic.getId(map));
+        }
+
+        // Remove all intrinsics
+        identityMap.keySet().removeAll(Intrinsic.DATA_FIELDS);
+
         // The entry is stored without the original ID, so add it back
-        map.keySet().removeAll(Intrinsic.DATA_FIELDS);
-        map.remove(MASKED_ID);
-        map.put(ID, id);
+        identityMap.remove(MASKED_ID);
+        identityMap.put(ID, id);
 
-        return JsonHelper.convert(map, _authIdentityClass);
+        return JsonHelper.convert(identityMap, _authIdentityClass);
     }
 
     @Override

quality/integration/src/test/java/test/integration/auth/TableAuthIdentityManagerTest.java

@@ -0,0 +1,117 @@
+package test.integration.auth;
+
+import com.bazaarvoice.emodb.auth.apikey.ApiKey;
+import com.bazaarvoice.emodb.auth.identity.TableAuthIdentityManager;
+import com.bazaarvoice.emodb.sor.api.AuditBuilder;
+import com.bazaarvoice.emodb.sor.api.DataStore;
+import com.bazaarvoice.emodb.sor.api.Intrinsic;
+import com.bazaarvoice.emodb.sor.core.test.InMemoryDataStore;
+import com.bazaarvoice.emodb.sor.delta.Deltas;
+import com.bazaarvoice.emodb.sor.uuid.TimeUUIDs;
+import com.codahale.metrics.MetricRegistry;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.hash.Hashing;
+import org.testng.annotations.Test;
+
+import java.util.Map;
+import java.util.Set;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotNull;
+
+public class TableAuthIdentityManagerTest {
+
+    /**
+     * There are two tables which store identities in TableAuthIdentityManager: One table keyed by a hash of the
+     * API key, and an index table ID'd by the internal ID which contains the API key hash.  This second table is used
+     * to look up API keys by internal ID.  It should be rare, but it is possible for an API key record to exist
+     * without a corresponding internal ID.  One possible way for this to happen is grandfathered in API keys
+     * created before the introduction of internal IDs.  TableAuthIdentityManager should rebuild the index
+     * when there is a missing or incorrect index record.  This test verifies that works as expected.
+     */
+    @Test
+    public void testRebuildInternalIdIndex() {
+        DataStore dataStore = new InMemoryDataStore(new MetricRegistry());
+        TableAuthIdentityManager<ApiKey> tableAuthIdentityManager = new TableAuthIdentityManager<>(
+                ApiKey.class, dataStore, "__auth:keys", "__auth:internal_ids", "app_global:sys", Hashing.sha256());
+
+        ApiKey apiKey = new ApiKey("testkey", "id0", ImmutableSet.of("role1", "role2"));
+        apiKey.setOwner("testowner");
+        tableAuthIdentityManager.updateIdentity(apiKey);
+
+        // Verify both tables have been written
+
+        String keyTableId = Hashing.sha256().hashUnencodedChars("testkey").toString();
+
+        Map<String, Object> keyMap = dataStore.get("__auth:keys", keyTableId);
+        assertFalse(Intrinsic.isDeleted(keyMap));
+        assertEquals(keyMap.get("owner"), "testowner");
+
+        Map<String, Object> indexMap = dataStore.get("__auth:internal_ids", "id0");
+        assertFalse(Intrinsic.isDeleted(indexMap));
+        assertEquals(indexMap.get("hashedId"), keyTableId);
+
+        // Deliberately delete the index map record
+        dataStore.update("__auth:internal_ids", "id0", TimeUUIDs.newUUID(), Deltas.delete(),
+                new AuditBuilder().setComment("test delete").build());
+
+        // Verify that a lookup by internal ID works
+        Set<String> roles = tableAuthIdentityManager.getRolesByInternalId("id0");
+        assertEquals(roles, ImmutableSet.of("role1", "role2"));
+
+        // Verify that the index record is re-created
+        indexMap = dataStore.get("__auth:internal_ids", "id0");
+        assertFalse(Intrinsic.isDeleted(indexMap));
+        assertEquals(indexMap.get("hashedId"), keyTableId);
+    }
+
+    @Test
+    public void testGrandfatheredInInternalId() {
+        DataStore dataStore = new InMemoryDataStore(new MetricRegistry());
+        TableAuthIdentityManager<ApiKey> tableAuthIdentityManager = new TableAuthIdentityManager<>(
+                ApiKey.class, dataStore, "__auth:keys", "__auth:internal_ids", "app_global:sys", Hashing.sha256());
+
+        // Perform an operation on tableAuthIdentityManager to force it to create API key tables; the actual
+        // operation doesn't matter.
+        tableAuthIdentityManager.getIdentity("ignore");
+
+        String id = "aaaabbbbccccddddeeeeffffgggghhhhiiiijjjjkkkkllll";
+        String hash = Hashing.sha256().hashUnencodedChars(id).toString();
+
+        // Write out a record which mimics the pre-internal-id format.  Notably missing is the "internalId" attribute.
+        Map<String, Object> oldIdentityMap = ImmutableMap.<String, Object>builder()
+                .put("maskedId", "aaaa****************************************llll")
+                .put("owner", "someone")
+                .put("description", "something")
+                .put("roles", ImmutableList.of("role1", "role2"))
+                .build();
+
+        dataStore.update("__auth:keys", hash, TimeUUIDs.newUUID(), Deltas.literal(oldIdentityMap),
+                new AuditBuilder().setComment("test grandfathering").build());
+
+        // Verify the record can be read by ID.  The key's internal ID will be the hashed ID.
+        ApiKey apiKey = tableAuthIdentityManager.getIdentity(id);
+        assertNotNull(apiKey);
+        assertEquals(apiKey.getId(), id);
+        assertEquals(apiKey.getInternalId(), hash);
+        assertEquals(apiKey.getOwner(), "someone");
+        assertEquals(apiKey.getDescription(), "something");
+        assertEquals(apiKey.getRoles(), ImmutableList.of("role1", "role2"));
+
+        // Verify that a lookup by internal ID works
+        Set<String> roles = tableAuthIdentityManager.getRolesByInternalId(hash);
+        assertEquals(roles, ImmutableSet.of("role1", "role2"));
+
+        // Verify that the index record was created with the hashed ID as the internal ID
+        Map<String, Object> indexMap = dataStore.get("__auth:internal_ids", hash);
+        assertFalse(Intrinsic.isDeleted(indexMap));
+        assertEquals(indexMap.get("hashedId"), hash);
+
+        // Verify lookup by internal ID still works with the index record in place
+        roles = tableAuthIdentityManager.getRolesByInternalId(hash);
+        assertEquals(roles, ImmutableSet.of("role1", "role2"));
+    }
+}