Changed API deletion to instead set key as inactive

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

@@ -1,6 +1,7 @@
 package com.bazaarvoice.emodb.auth.apikey;
 
 import com.bazaarvoice.emodb.auth.identity.AuthIdentity;
+import com.bazaarvoice.emodb.auth.identity.IdentityState;
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.collect.ImmutableSet;
@@ -13,15 +14,15 @@
  */
 public class ApiKey extends AuthIdentity {
 
-    public ApiKey(String key, String internalId, Set<String> roles) {
-        super(key, internalId, roles);
+    public ApiKey(String key, String internalId, IdentityState state, Set<String> roles) {
+        super(key, internalId, state, roles);
     }
 
     @JsonCreator
     public ApiKey(@JsonProperty("id") String key,
                   @JsonProperty("internalId") String internalId,
+                  @JsonProperty("state") IdentityState state,
                   @JsonProperty("roles") List<String> roles) {
-
-        this(key, internalId, ImmutableSet.copyOf(roles));
+        this(key, internalId, state, ImmutableSet.copyOf(roles));
     }
 }

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

@@ -1,6 +1,7 @@
 package com.bazaarvoice.emodb.auth.apikey;
 
 import com.bazaarvoice.emodb.auth.identity.AuthIdentityManager;
+import com.bazaarvoice.emodb.auth.identity.InternalIdentity;
 import com.bazaarvoice.emodb.auth.permissions.PermissionManager;
 import com.bazaarvoice.emodb.auth.shiro.AnonymousCredentialsMatcher;
 import com.bazaarvoice.emodb.auth.shiro.AnonymousToken;
@@ -211,7 +212,9 @@ protected AuthenticationInfo doGetAuthenticationInfo(AuthenticationToken token)
      */
     private AuthenticationInfo getUncachedAuthenticationInfoForKey(String id) {
         ApiKey apiKey = _authIdentityManager.getIdentity(id);
-        if (apiKey == null) {
+
+        // If the API key exists but cannot be authenticated then return null
+        if (apiKey == null || !apiKey.getState().isActive()) {
             return null;
         }
 
@@ -416,14 +419,15 @@ private void cacheAuthorizationInfoByInternalId(String internalId, Authorization
      * Gets the authorization info for an API key's internal ID from the source (not from cache).
      */
     private AuthorizationInfo getUncachedAuthorizationInfoByInternalId(String internalId) {
-        // Retrieve the roles by internal ID
-        Set<String> roles = _authIdentityManager.getRolesByInternalId(internalId);
-        if (roles == null) {
-            _log.debug("Authorization info requested for non-existent internal id {}", internalId);
+        // Retrieve the internal identity
+        InternalIdentity identity = _authIdentityManager.getInternalIdentity(internalId);
+        if (identity == null || !identity.getState().isActive()) {
+            _log.debug("Authorization info requested for {} internal id {}",
+                    identity == null ? "non-existent" : identity.getState().toString().toLowerCase(), internalId);
             return _nullAuthorizationInfo;
         }
 
-        return new SimpleAuthorizationInfo(ImmutableSet.copyOf(roles));
+        return new SimpleAuthorizationInfo(ImmutableSet.copyOf(identity.getRoles()));
     }
 
     /**

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/AuthIdentity.java

@@ -12,7 +12,7 @@
 /**
  * Base unit of identity for a client which can be authenticated in the application.
  */
-abstract public class AuthIdentity {
+abstract public class AuthIdentity implements InternalIdentity {
 
     /**
      * Each identity is associated with an internal ID which is never exposed outside the system.  This is done
@@ -49,28 +49,33 @@ abstract public class AuthIdentity {
     private String _description;
     // Date this identity was issued
     private Date _issued;
+    // State for this identity
+    private IdentityState _state;
 
-
-    protected AuthIdentity(String id, String internalId, Set<String> roles) {
+    protected AuthIdentity(String id, String internalId, IdentityState state, Set<String> roles) {
         checkArgument(!Strings.isNullOrEmpty(id), "id");
         checkArgument(!Strings.isNullOrEmpty(internalId), "internalId");
         _id = id;
         _internalId = internalId;
+        _state = state;
         _roles = ImmutableSet.copyOf(checkNotNull(roles, "roles"));
     }
 
     public String getId() {
         return _id;
     }
 
+    @Override
     public String getInternalId() {
         return _internalId;
     }
 
+    @Override
     public Set<String> getRoles() {
         return _roles;
     }
 
+    @Override
     public String getOwner() {
         return _owner;
     }
@@ -79,6 +84,7 @@ public void setOwner(String owner) {
         _owner = owner;
     }
 
+    @Override
     public String getDescription() {
         return _description;
     }
@@ -87,11 +93,21 @@ public void setDescription(String description) {
         _description = description;
     }
 
+    @Override
     public Date getIssued() {
         return _issued;
     }
 
     public void setIssued(Date issued) {
         _issued = issued;
     }
+
+    @Override
+    public IdentityState getState() {
+        return _state;
+    }
+
+    public void setState(IdentityState state) {
+        _state = state;
+    }
 }

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/AuthIdentityManager.java

@@ -1,7 +1,5 @@
 package com.bazaarvoice.emodb.auth.identity;
 
-import java.util.Set;
-
 /**
  * Manager for identities.
  */
@@ -18,12 +16,34 @@ public interface AuthIdentityManager<T extends AuthIdentity> {
     void updateIdentity(T identity);
 
     /**
-     * Deletes an identity.
+     * Migrates an existing identity to a new identity.  The internal ID for the identity remains unchanged.
+     * This is useful if an identity has been compromised and the secret ID needs to be changed while keeping all other
+     * attributes of the identity unmodified.
+     */
+    void migrateIdentity(String existingId, String newId);
+
+    /**
+     * Deletes an identity.  This should NOT be called under normal circumstances and is in place to support internal
+     * testing.  This method deletes any record that the identity exists, leaving open the following possible vectors:
+     *
+     * <ol>
+     *     <li>The ID can be recreated, allowing the previous user to access using the ID.</li>
+     *     <li>A typical implementation stores the identity using a hash of the ID.  If another identity is created
+     *         whose ID hashes the same value then the previous users's ID would authenticate as the new ID.</li>
+     *     <li>As more of the system associates first order objects with owners there is an increased risk for
+     *         dangling identity references if they are deleted.</li>
+     * </ol>
+     *
+     * Under normal circumstances the correct approach is to set an identity's state to INACTIVE using
+     * {@link #updateIdentity(AuthIdentity)} rather than to delete the identity.
      */
-    void deleteIdentity(String id);
+    void deleteIdentityUnsafe(String id);
 
     /**
-     * Gets the roles associated with an identity by its internal ID.
+     * Returns an {@link InternalIdentity} view of the identity identified by internal ID, or null if no
+     * such entity exists.  This method is useful when internal operations are required for a user, such as
+     * verifying whether she has specific permissions, without actually logging the user in or exposing sufficient
+     * information for the caller to log the user in or spoof that user's identity.
      */
-    Set<String> getRolesByInternalId(String internalId);
+    InternalIdentity getInternalIdentity(String internalId);
 }

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/IdentityState.java

@@ -0,0 +1,27 @@
+package com.bazaarvoice.emodb.auth.identity;
+
+/**
+ * State for an {@link AuthIdentity}.  Currently there are 3 possible values:
+ *
+ * <ol>
+ *     <li>ACTIVE.  This is the normal state for an identity.</li>
+ *     <li>INACTIVE.  The identity exists but cannot be authenticated or authorized for any operations.</li>
+ *     <li>MIGRATED.  The identity's ID has been migrated and the current identity is a historical record from the
+ *                    old ID.  Like INACTIVE, a MIGRATED identity cannot be authenticated or authorized.</li>
+ * </ol>
+ */
+public enum IdentityState {
+    ACTIVE,
+    INACTIVE,
+    MIGRATED;
+
+    /**
+     * Returns true if an identity is in a state where it can be authorized or authenticated.  The current
+     * implementation redundantly returns true only for ACTIVE.  Even so, use of this method is preferred to
+     * verify if an identity can be authorized or authenticated since that tautology may change if other states
+     * are introduced.
+     */
+    public boolean isActive() {
+        return this == ACTIVE;
+    }
+}

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/InMemoryAuthIdentityManager.java

@@ -1,9 +1,10 @@
 package com.bazaarvoice.emodb.auth.identity;
 
+import com.bazaarvoice.emodb.common.json.JsonHelper;
+import com.fasterxml.jackson.core.type.TypeReference;
 import com.google.common.collect.Maps;
 
 import java.util.Map;
-import java.util.Set;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
@@ -12,8 +13,13 @@
  */
 public class InMemoryAuthIdentityManager<T extends AuthIdentity> implements AuthIdentityManager<T> {
 
+    private final Class<T> _identityClass;
     private final Map<String, T> _identityMap = Maps.newConcurrentMap();
 
+    public InMemoryAuthIdentityManager(Class<T> identityClass) {
+        _identityClass = checkNotNull(identityClass, "identityClass");
+    }
+
     @Override
     public T getIdentity(String id) {
         checkNotNull(id, "id");
@@ -28,17 +34,33 @@ public void updateIdentity(T identity) {
     }
 
     @Override
-    public void deleteIdentity(String id) {
+    public void migrateIdentity(String existingId, String newId) {
+        T identity = getIdentity(existingId);
+        if (identity != null) {
+            // Use JSON serialization to create a copy.
+            Map<String, Object> newIdentityMap = JsonHelper.convert(identity, new TypeReference<Map<String, Object>>() {});
+            // Change the ID to the new ID.
+            newIdentityMap.put("id", newId);
+            T newIdentity = JsonHelper.convert(newIdentityMap, _identityClass);
+            _identityMap.put(newId, newIdentity);
+
+            // Change the state of the existing identity to migrated
+            identity.setState(IdentityState.MIGRATED);
+        }
+    }
+
+    @Override
+    public void deleteIdentityUnsafe(String id) {
         checkNotNull(id, "id");
         _identityMap.remove(id);
     }
 
     @Override
-    public Set<String> getRolesByInternalId(String internalId) {
+    public InternalIdentity getInternalIdentity(String internalId) {
         checkNotNull(internalId, "internalId");
         for (T identity : _identityMap.values()) {
             if (internalId.equals(identity.getInternalId())) {
-                return identity.getRoles();
+                return identity;
             }
         }
         return null;

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/InternalIdentity.java

@@ -0,0 +1,25 @@
+package com.bazaarvoice.emodb.auth.identity;
+
+import java.util.Date;
+import java.util.Set;
+
+/**
+ * Interface for getting information about an identity for internal purposes.  Notably it this interface does not
+ * expose the secret ID of the identity, such as its API key.  This allows use of the identity for validating
+ * permissions and other common metadata without exposing the ID necessary for logging in, spoofing, or otherwise
+ * leaking the identity.
+ */
+public interface InternalIdentity {
+
+    String getInternalId();
+
+    Set<String> getRoles();
+
+    String getOwner();
+
+    String getDescription();
+
+    Date getIssued();
+
+    IdentityState getState();
+}

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

@@ -2,8 +2,6 @@
 
 import com.bazaarvoice.emodb.auth.shiro.InvalidatableCacheManager;
 
-import java.util.Set;
-
 import static com.google.common.base.Preconditions.checkNotNull;
 
 /**
@@ -33,15 +31,23 @@ public void updateIdentity(T identity) {
     }
 
     @Override
-    public void deleteIdentity(String id) {
-        checkNotNull(id);
-        _manager.deleteIdentity(id);
+    public void migrateIdentity(String existingId, String newId) {
+        checkNotNull(existingId);
+        checkNotNull(newId);
+        _manager.migrateIdentity(existingId, newId);
+        _cacheManager.invalidateAll();
+    }
+
+    @Override
+    public void deleteIdentityUnsafe(String id) {
+        checkNotNull(id, "id");
+        _manager.deleteIdentityUnsafe(id);
         _cacheManager.invalidateAll();
     }
 
     @Override
-    public Set<String> getRolesByInternalId(String internalId) {
+    public InternalIdentity getInternalIdentity(String internalId) {
         checkNotNull(internalId, "internalId");
-        return _manager.getRolesByInternalId(internalId);
+        return _manager.getInternalIdentity(internalId);
     }
 }

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

@@ -1,14 +1,10 @@
 package com.bazaarvoice.emodb.auth.identity;
 
-import com.google.common.base.Function;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.Maps;
 
 import javax.annotation.Nullable;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
@@ -64,20 +60,29 @@ public void updateIdentity(T identity) {
     }
 
     @Override
-    public void deleteIdentity(String id) {
-        checkNotNull(id);
+    public void migrateIdentity(String existingId, String newId) {
+        checkNotNull(existingId);
+        checkNotNull(newId);
+        checkArgument(!_identityMap.containsKey(existingId), "Cannot migrate from static identity: %s", existingId);
+        checkArgument(!_identityMap.containsKey(newId), "Cannot migrate to static identity: %s", newId);
+        _manager.migrateIdentity(existingId, newId);
+    }
+
+    @Override
+    public void deleteIdentityUnsafe(String id) {
+        checkNotNull(id, "id");
         checkArgument(!_identityMap.containsKey(id), "Cannot delete static identity: %s", id);
-        _manager.deleteIdentity(id);
+        _manager.deleteIdentityUnsafe(id);
     }
 
     @Override
-    public Set<String> getRolesByInternalId(String internalId) {
+    public InternalIdentity getInternalIdentity(String internalId) {
         checkNotNull(internalId, "internalId");
 
         T identity = _internalIdMap.get(internalId);
         if (identity != null) {
-            return identity.getRoles();
+            return identity;
         }
-        return _manager.getRolesByInternalId(internalId);
+        return _manager.getInternalIdentity(internalId);
     }
 }