diff --git a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/apikey/ApiKey.java b/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/apikey/ApiKey.java index ee913f1379..5d609fba66 100644 --- a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/apikey/ApiKey.java +++ b/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 roles) { - super(key, internalId, roles); + public ApiKey(String key, String internalId, IdentityState state, Set roles) { + super(key, internalId, state, roles); } @JsonCreator public ApiKey(@JsonProperty("id") String key, @JsonProperty("internalId") String internalId, + @JsonProperty("state") IdentityState state, @JsonProperty("roles") List roles) { - - this(key, internalId, ImmutableSet.copyOf(roles)); + this(key, internalId, state, ImmutableSet.copyOf(roles)); } } diff --git a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/apikey/ApiKeyRealm.java b/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/apikey/ApiKeyRealm.java index 3b0bf6641b..aeafc79534 100644 --- a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/apikey/ApiKeyRealm.java +++ b/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 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())); } /** diff --git a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/AuthIdentity.java b/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/AuthIdentity.java index 8ffa243ee6..6313385a48 100644 --- a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/AuthIdentity.java +++ b/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,13 +49,15 @@ 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 roles) { + protected AuthIdentity(String id, String internalId, IdentityState state, Set roles) { checkArgument(!Strings.isNullOrEmpty(id), "id"); checkArgument(!Strings.isNullOrEmpty(internalId), "internalId"); _id = id; _internalId = internalId; + _state = state; _roles = ImmutableSet.copyOf(checkNotNull(roles, "roles")); } @@ -63,14 +65,17 @@ public String getId() { return _id; } + @Override public String getInternalId() { return _internalId; } + @Override public Set 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,6 +93,7 @@ public void setDescription(String description) { _description = description; } + @Override public Date getIssued() { return _issued; } @@ -94,4 +101,13 @@ public Date getIssued() { public void setIssued(Date issued) { _issued = issued; } + + @Override + public IdentityState getState() { + return _state; + } + + public void setState(IdentityState state) { + _state = state; + } } diff --git a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/AuthIdentityManager.java b/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/AuthIdentityManager.java index 7ff3b7535b..1fc4744284 100644 --- a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/AuthIdentityManager.java +++ b/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 { 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: + * + *
    + *
  1. The ID can be recreated, allowing the previous user to access using the ID.
    2. + *
  2. 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.
    3. + *
  3. As more of the system associates first order objects with owners there is an increased risk for + * dangling identity references if they are deleted.
    4. + *
+ * + * 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 getRolesByInternalId(String internalId); + InternalIdentity getInternalIdentity(String internalId); } diff --git a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/IdentityState.java b/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/IdentityState.java new file mode 100644 index 0000000000..816be44e85 --- /dev/null +++ b/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: + * + *
    + *
  1. ACTIVE. This is the normal state for an identity.
    2. + *
  2. INACTIVE. The identity exists but cannot be authenticated or authorized for any operations.
    3. + *
  3. 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.
    4. + *
+ */ +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; + } +} diff --git a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/InMemoryAuthIdentityManager.java b/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/InMemoryAuthIdentityManager.java index e6a65631ad..f1251bf9dc 100644 --- a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/InMemoryAuthIdentityManager.java +++ b/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 implements AuthIdentityManager { + private final Class _identityClass; private final Map _identityMap = Maps.newConcurrentMap(); + public InMemoryAuthIdentityManager(Class 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 newIdentityMap = JsonHelper.convert(identity, new TypeReference>() {}); + // 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 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; diff --git a/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/InternalIdentity.java b/auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/InternalIdentity.java new file mode 100644 index 0000000000..41a3bc5ece --- /dev/null +++ b/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 getRoles(); + + String getOwner(); + + String getDescription(); + + Date getIssued(); + + IdentityState getState(); +} diff --git a/auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/CacheManagingAuthIdentityManager.java b/auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/CacheManagingAuthIdentityManager.java index 0f7a2be3a1..9b3ce97ea1 100644 --- a/auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/CacheManagingAuthIdentityManager.java +++ b/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 getRolesByInternalId(String internalId) { + public InternalIdentity getInternalIdentity(String internalId) { checkNotNull(internalId, "internalId"); - return _manager.getRolesByInternalId(internalId); + return _manager.getInternalIdentity(internalId); } } diff --git a/auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/DeferringAuthIdentityManager.java b/auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/DeferringAuthIdentityManager.java index 65d7cc482a..a1a0afe231 100644 --- a/auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/DeferringAuthIdentityManager.java +++ b/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 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); } } diff --git a/auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/TableAuthIdentityManager.java b/auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/TableAuthIdentityManager.java index aaee261d15..f54d9fa361 100644 --- a/auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/TableAuthIdentityManager.java +++ b/auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/identity/TableAuthIdentityManager.java @@ -10,6 +10,7 @@ import com.bazaarvoice.emodb.sor.api.TableOptionsBuilder; import com.bazaarvoice.emodb.sor.api.Update; import com.bazaarvoice.emodb.sor.api.WriteConsistency; +import com.bazaarvoice.emodb.sor.condition.Condition; import com.bazaarvoice.emodb.sor.condition.Conditions; import com.bazaarvoice.emodb.sor.delta.Delta; import com.bazaarvoice.emodb.sor.delta.Deltas; @@ -17,13 +18,14 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.hash.HashFunction; import javax.annotation.Nullable; import java.util.Iterator; +import java.util.List; import java.util.Map; -import java.util.Set; import java.util.UUID; import static com.google.common.base.Preconditions.checkArgument; @@ -41,6 +43,7 @@ public class TableAuthIdentityManager implements AuthIde private final static String INTERNAL_ID = "internalId"; private final static String MASKED_ID = "maskedId"; private final static String HASHED_ID = "hashedId"; + private final static String STATE = "state"; private final Class _authIdentityClass; private final DataStore _dataStore; @@ -78,6 +81,7 @@ public T getIdentity(String id) { } private T convertDataStoreEntryToIdentity(String id, Map map) { + // If the record does not exist or has been deleted then return null if (map == null || Intrinsic.isDeleted(map)) { return null; } @@ -90,6 +94,10 @@ private T convertDataStoreEntryToIdentity(String id, Map map) { if (!identityMap.containsKey(INTERNAL_ID)) { identityMap.put(INTERNAL_ID, Intrinsic.getId(map)); } + // Similarly grandfather in identities with no state attribute as active + if (!identityMap.containsKey(STATE)) { + identityMap.put(STATE, IdentityState.ACTIVE.toString()); + } // Remove all intrinsics identityMap.keySet().removeAll(Intrinsic.DATA_FIELDS); @@ -103,51 +111,105 @@ private T convertDataStoreEntryToIdentity(String id, Map map) { @Override public void updateIdentity(T identity) { + updateOrMigrateIdentity(identity, null); + } + + @Override + public void migrateIdentity(String existingId, String newId) { + checkNotNull(existingId, "existingId"); + checkNotNull(newId, "newId"); + + if (existingId.equals(newId)) { + // Trivial case. Don't throw an exception, just return without performing any actual updates. + return; + } + + T identity = getIdentity(existingId); + checkNotNull(identity, "Unknown identity: %s", existingId); + + updateOrMigrateIdentity(identity, newId); + } + + private void updateOrMigrateIdentity(T identity, @Nullable String newId) { + boolean isUpdate = newId == null; + checkNotNull(identity, "identity"); String id = checkNotNull(identity.getId(), "id"); String internalId = checkNotNull(identity.getInternalId(), "internalId"); + checkArgument(identity.getState().isActive() || isUpdate, "Cannot migrate %s identity", identity.getState()); + validateTables(); + List updates = Lists.newArrayListWithCapacity(3); + UUID changeId = TimeUUIDs.newUUID(); - Audit audit = new AuditBuilder().setLocalHost().setComment("update identity").build(); + Audit audit = new AuditBuilder() + .setLocalHost() + .setComment(String.format("%s identity", isUpdate ? "update" : "migrate")) + .build(); String hashedId = hash(id); - Map map = JsonHelper.convert(identity, new TypeReference>(){}); + String maskedId; + Map identityMap = JsonHelper.convert(identity, new TypeReference>(){}); + + if (isUpdate) { + maskedId = mask(id); + } else { + // Change the state for the existing identity to migrated. Use JSON serialization to create a clone + // of the original identity + T oldIdentity = JsonHelper.convert(identityMap, _authIdentityClass); + // Set the state to migrated + oldIdentity.setState(IdentityState.MIGRATED); + // Convert the old identity back to a delta Map representation + Map oldIdentityMap = JsonHelper.convert(oldIdentity, new TypeReference>(){}); + // Strip the ID so it doesn't get persisted, then update the remaining attributes + oldIdentityMap.remove(ID); + Delta delta = Deltas.mapBuilder().putAll(oldIdentityMap).build(); + updates.add(new Update(_identityTableName, hashedId, changeId, delta, audit, WriteConsistency.GLOBAL)); + + // Update the ID and masked ID to match the new identity + hashedId = hash(newId); + maskedId = mask(newId); + } // Strip the ID and replace it with a masked version - map.remove(ID); - map.put(MASKED_ID, mask(id)); + identityMap.remove(ID); + identityMap.put(MASKED_ID, maskedId); - Update identityUpdate = new Update(_identityTableName, hashedId, changeId, Deltas.literal(map), audit, WriteConsistency.GLOBAL); + updates.add(new Update(_identityTableName, hashedId, changeId, Deltas.literal(identityMap), audit, WriteConsistency.GLOBAL)); - map = ImmutableMap.of(HASHED_ID, hashedId); - Update internalIdUpdate = new Update(_internalIdIndexTableName, internalId, changeId, Deltas.literal(map), audit, WriteConsistency.GLOBAL); + Map internalIdMap = ImmutableMap.of(HASHED_ID, hashedId); + updates.add(new Update(_internalIdIndexTableName, internalId, changeId, Deltas.literal(internalIdMap), audit, WriteConsistency.GLOBAL)); - // Update the identity and internal ID index in a single update - _dataStore.updateAll(ImmutableList.of(identityUpdate, internalIdUpdate)); + // Update the identity record(s) and internal ID index in a single update + _dataStore.updateAll(updates); } @Override - public void deleteIdentity(String id) { + public void deleteIdentityUnsafe(String id) { checkNotNull(id, "id"); - validateTables(); - String hashedId = hash(id); + T identity = getIdentity(id); + if (identity == null) { + // Identity did not exist. Don't raise an exception, just silently return with no action. + return; + } - _dataStore.update( - _identityTableName, - hashedId, - TimeUUIDs.newUUID(), - Deltas.delete(), - new AuditBuilder().setLocalHost().setComment("delete identity").build(), - WriteConsistency.GLOBAL); + // Create two updates; one to delete from the identity table and one to delete from the internal ID + // index table - // Don't delete the entry from the internal ID index table; it will be lazily deleted next time it is used. - // Otherwise there may be a race condition when an API key is migrated. + UUID changeId = TimeUUIDs.newUUID(); + Audit audit = new AuditBuilder().setComment("delete identity").setLocalHost().build(); + + List updates = ImmutableList.of( + new Update(_identityTableName, hash(id), changeId, Deltas.delete(), audit, WriteConsistency.GLOBAL), + new Update(_internalIdIndexTableName, identity.getInternalId(), changeId, Deltas.delete(), audit, WriteConsistency.GLOBAL)); + + _dataStore.updateAll(updates); } @Override - public Set getRolesByInternalId(String internalId) { + public InternalIdentity getInternalIdentity(String internalId) { checkNotNull(internalId, "internalId"); // The actual ID is stored using a one-way hash so it is not recoverable. Use a dummy value to satisfy @@ -155,6 +217,7 @@ public Set getRolesByInternalId(String internalId) { final String STUB_ID = "ignore"; T identity = null; + String staleHashedId = null; // First try using the index table to determine the hashed ID. Map internalIdRecord = _dataStore.get(_internalIdIndexTableName, internalId); @@ -167,45 +230,61 @@ public Set getRolesByInternalId(String internalId) { // Disregard the value identity = null; - // The internal ID index table entry was stale. Delete it. - Delta deleteIndexRecord = Deltas.conditional( - Conditions.mapBuilder() - .matches(HASHED_ID, Conditions.equal(hashedId)) - .build(), - Deltas.delete()); - - _dataStore.update(_internalIdIndexTableName, internalId, TimeUUIDs.newUUID(), deleteIndexRecord, - new AuditBuilder().setLocalHost().setComment("delete stale identity").build()); + // The internal ID index table entry was stale. Save the ID for later update or deletion. + staleHashedId = hashedId; } } if (identity == null) { // This should be rare, but if the record was not found or was stale in the index table then scan for it. + String hashedId = null; Iterator> entries = _dataStore.scan(_identityTableName, null, Long.MAX_VALUE, ReadConsistency.STRONG); - while (entries.hasNext() && identity == null) { + while (entries.hasNext() && (identity == null || identity.getState() == IdentityState.MIGRATED)) { Map entry = entries.next(); T potentialIdentity = convertDataStoreEntryToIdentity(STUB_ID, entry); if (potentialIdentity != null && internalId.equals(potentialIdentity.getInternalId())) { - // We found the identity + // We potentially found the identity. If it is migrated then we'll keep searching for an identity + // that is not migrated. identity = potentialIdentity; - - // Update the internal ID index. There is a possible race condition if the identity is being - // migrated concurrent to this update. If that happens, however, the next time it is read the - // index will be incorrect and it will be lazily updated at that time. - Delta updateIndexRecord = Deltas.literal(ImmutableMap.of(HASHED_ID, Intrinsic.getId(entry))); - _dataStore.update(_internalIdIndexTableName, internalId, TimeUUIDs.newUUID(), updateIndexRecord, - new AuditBuilder().setLocalHost().setComment("update identity").build()); + hashedId = Intrinsic.getId(entry); } } - } - if (identity != null) { - return identity.getRoles(); + if (hashedId != null || staleHashedId != null) { + // Update the internal ID index. There is a possible race condition if the identity is being + // migrated concurrent to this update. For that reason the index update will be predicated on the + // condition that the value is the same as what was previously read. + + Condition condition; + Delta delta; + + if (staleHashedId == null) { + // Only update the index if there is still no index record + condition = Conditions.isUndefined(); + } else { + // Only update the index if it still has the same stale value + condition = Conditions.mapBuilder() + .matches(HASHED_ID, Conditions.equal(staleHashedId)) + .build(); + } + + if (hashedId == null) { + // Delete the index record if it doesn't map to an identity + delta = Deltas.delete(); + } else { + // Update the index record with the hashed ID of the identity + delta = Deltas.literal(ImmutableMap.of(HASHED_ID, hashedId)); + } + + _dataStore.update(_internalIdIndexTableName, internalId, TimeUUIDs.newUUID(), + Deltas.conditional(condition, delta), + new AuditBuilder().setLocalHost().setComment("update identity").build(), WriteConsistency.GLOBAL); + } } - // Identity not found, return null - return null; + // If this identity was found this will be not null, otherwise it returns null signalling it was not found. + return identity; } private void validateTables() { diff --git a/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/CachingTest.java b/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/CachingTest.java index 25f1878e1c..68ad5a5f80 100644 --- a/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/CachingTest.java +++ b/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/CachingTest.java @@ -4,6 +4,7 @@ import com.bazaarvoice.emodb.auth.apikey.ApiKeyRequest; import com.bazaarvoice.emodb.auth.identity.AuthIdentityManager; import com.bazaarvoice.emodb.auth.identity.CacheManagingAuthIdentityManager; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.identity.InMemoryAuthIdentityManager; import com.bazaarvoice.emodb.auth.jersey.Authenticated; import com.bazaarvoice.emodb.auth.jersey.Subject; @@ -76,7 +77,7 @@ protected ResourceTestRule setupResourceTestRule() { CacheRegistry cacheRegistry = new DefaultCacheRegistry(new SimpleLifeCycleRegistry(), new MetricRegistry()); _cacheManager = new GuavaCacheManager(cacheRegistry); - InMemoryAuthIdentityManager authIdentityDAO = new InMemoryAuthIdentityManager<>(); + InMemoryAuthIdentityManager authIdentityDAO = new InMemoryAuthIdentityManager<>(ApiKey.class); _authIdentityCaching = new CacheManagingAuthIdentityManager<>(authIdentityDAO, _cacheManager); _authIdentityManager = spy(_authIdentityCaching); @@ -84,8 +85,8 @@ protected ResourceTestRule setupResourceTestRule() { _permissionCaching = new CacheManagingPermissionManager(permissionDAO, _cacheManager); _permissionManager = spy(_permissionCaching); - authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", ImmutableSet.of("testrole"))); - authIdentityDAO.updateIdentity(new ApiKey("othertestkey", "id1", ImmutableSet.of("testrole"))); + authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", IdentityState.ACTIVE, ImmutableSet.of("testrole"))); + authIdentityDAO.updateIdentity(new ApiKey("othertestkey", "id1", IdentityState.ACTIVE, ImmutableSet.of("testrole"))); permissionDAO.updateForRole("testrole", new PermissionUpdateRequest().permit("city|get|Madrid", "country|get|Spain")); @@ -140,7 +141,7 @@ public void testInvalidateByMutation() throws Exception { _permissionCaching.updateForRole("othertestrole", new PermissionUpdateRequest().permit("city|get|Austin", "country|get|USA")); testGetWithMatchingPermissions("testkey", "Spain", "Madrid"); testGetWithMatchingPermissions("testkey", "Spain", "Madrid"); - _authIdentityCaching.updateIdentity(new ApiKey("othertestkey", "id1", ImmutableSet.of("othertestrole"))); + _authIdentityCaching.updateIdentity(new ApiKey("othertestkey", "id1", IdentityState.ACTIVE, ImmutableSet.of("othertestrole"))); testGetWithMatchingPermissions("testkey", "Spain", "Madrid"); testGetWithMatchingPermissions("testkey", "Spain", "Madrid"); verify(_authIdentityManager, times(6)).getIdentity("testkey"); @@ -173,7 +174,7 @@ public void testAddUserAndPermissions() throws Exception { testGetWithMissingPermissions("othertestkey", "USA", "Austin"); testGetWithMissingPermissions("othertestkey", "USA", "Austin"); _permissionCaching.updateForRole("othertestrole", new PermissionUpdateRequest().permit("city|get|Austin", "country|get|USA")); - _authIdentityCaching.updateIdentity(new ApiKey("othertestkey", "id1", ImmutableSet.of("testrole", "othertestrole"))); + _authIdentityCaching.updateIdentity(new ApiKey("othertestkey", "id1", IdentityState.ACTIVE, ImmutableSet.of("testrole", "othertestrole"))); testGetWithMatchingPermissions("othertestkey", "USA", "Austin"); // +1 othertestkey, +1 othertestrole testGetWithMatchingPermissions("othertestkey", "USA", "Austin"); diff --git a/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/ResourcePermissionsTest.java b/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/ResourcePermissionsTest.java index e5781b4a4d..f8305536eb 100644 --- a/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/ResourcePermissionsTest.java +++ b/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/ResourcePermissionsTest.java @@ -2,6 +2,7 @@ import com.bazaarvoice.emodb.auth.apikey.ApiKey; import com.bazaarvoice.emodb.auth.apikey.ApiKeyRequest; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.identity.InMemoryAuthIdentityManager; import com.bazaarvoice.emodb.auth.jersey.Authenticated; import com.bazaarvoice.emodb.auth.jersey.Subject; @@ -9,7 +10,7 @@ import com.bazaarvoice.emodb.auth.permissions.MatchingPermissionResolver; import com.bazaarvoice.emodb.auth.permissions.PermissionUpdateRequest; import com.bazaarvoice.emodb.auth.test.ResourceTestAuthUtil; -import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.sun.jersey.api.client.ClientResponse; import com.sun.jersey.api.client.WebResource; @@ -19,6 +20,8 @@ import org.junit.After; import org.junit.Rule; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; import javax.ws.rs.GET; import javax.ws.rs.Path; @@ -29,24 +32,53 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import java.net.URLEncoder; -import java.util.Map; +import java.util.Collection; import static com.bazaarvoice.emodb.auth.permissions.MatchingPermission.escape; import static java.lang.String.format; import static org.testng.Assert.assertEquals; +@RunWith(Parameterized.class) public class ResourcePermissionsTest { - private InMemoryAuthIdentityManager _authIdentityDAO = new InMemoryAuthIdentityManager<>(); + private InMemoryAuthIdentityManager _authIdentityDAO = new InMemoryAuthIdentityManager<>(ApiKey.class); private InMemoryPermissionManager _permissionDAO = new InMemoryPermissionManager(new MatchingPermissionResolver()); + private PermissionCheck _permissionCheck; + + public ResourcePermissionsTest(PermissionCheck permissionCheck) { + _permissionCheck = permissionCheck; + } + @Rule public ResourceTestRule _resourceTestRule = setupResourceTestRule(); + /** + * Run each test using an endpoint that either uses an explicit permission check, an implicit permission check based + * on path values, or an implicit permission check based on query parameters. + */ private enum PermissionCheck { - EXPLICIT, - PATH, - QUERY + EXPLICIT("/explicit/country/%s/city/%s"), + PATH("/path/country/%s/city/%s"), + QUERY("/query/welcome?country=%s&city=%s"); + + private String uriFormat; + + PermissionCheck(String format) { + this.uriFormat = format; + } + + public String getUri(String country, String city) throws Exception { + return format(uriFormat, URLEncoder.encode(country, "UTF-8"), URLEncoder.encode(city, "UTF-8")); + } + } + + @Parameterized.Parameters + public static Collection permissionChecks() { + return ImmutableList.of( + new Object[] { PermissionCheck.EXPLICIT }, + new Object[] { PermissionCheck.PATH }, + new Object[] { PermissionCheck.QUERY }); } @Path ("explicit/country/{country}") @@ -109,216 +141,100 @@ public void cleanupTest() { } @Test - public void testGetWithMissingIdentityExplicit() throws Exception { - testGetWithMissingIdentity(PermissionCheck.EXPLICIT); - } - - @Test - public void testGetWithMissingIdentityPath() throws Exception { - testGetWithMissingIdentity(PermissionCheck.PATH); - } - - @Test - public void testGetWithMissingIdentityQuery() throws Exception { - testGetWithMissingIdentity(PermissionCheck.QUERY); - } - - private void testGetWithMissingIdentity(PermissionCheck permissionCheck) throws Exception { - ClientResponse response = getCountryAndCity(permissionCheck, "Spain", "Madrid", null); + public void testGetWithMissingIdentity() throws Exception { + ClientResponse response = getCountryAndCity("Spain", "Madrid", null); assertEquals(response.getStatus(), Response.Status.FORBIDDEN.getStatusCode()); } @Test - public void testGetWithNonExistentIdentityExplicit() throws Exception { - testGetWithNonExistentIdentity(PermissionCheck.EXPLICIT); - } - - @Test - public void testGetWithNonExistentIdentityPath() throws Exception { - testGetWithNonExistentIdentity(PermissionCheck.PATH); - } - - @Test - public void testGetWithNonExistentIdentityQuery() throws Exception { - testGetWithNonExistentIdentity(PermissionCheck.QUERY); - } - - private void testGetWithNonExistentIdentity(PermissionCheck permissionCheck) throws Exception { - ClientResponse response = getCountryAndCity(permissionCheck, "Spain", "Madrid", "testkey"); + public void testGetWithNonExistentIdentity() throws Exception { + ClientResponse response = getCountryAndCity("Spain", "Madrid", "testkey"); assertEquals(response.getStatus(), Response.Status.FORBIDDEN.getStatusCode()); } @Test - public void testGetWithMissingPermissionExplicit() throws Exception { - testGetWithMissingPermission(PermissionCheck.EXPLICIT); - } - - @Test - public void testGetWithMissingPermissionPath() throws Exception { - testGetWithMissingPermission(PermissionCheck.PATH); - } - - @Test - public void testGetWithMissingPermissionQuery() throws Exception { - testGetWithMissingPermission(PermissionCheck.QUERY); - } - - private void testGetWithMissingPermission(PermissionCheck permissionCheck) throws Exception { - _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", ImmutableSet.of("testrole"))); + public void testGetWithMissingPermission() throws Exception { + _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", IdentityState.ACTIVE, ImmutableSet.of("testrole"))); _permissionDAO.updateForRole("testrole", new PermissionUpdateRequest().permit("country|get|Spain")); - ClientResponse response = getCountryAndCity(permissionCheck, "Spain", "Madrid", "testkey"); + ClientResponse response = getCountryAndCity("Spain", "Madrid", "testkey"); assertEquals(response.getStatus(), Response.Status.FORBIDDEN.getStatusCode()); } @Test - public void testGetWithMatchingPermissionsExplicit() throws Exception { - testGetWithMatchingPermissions(PermissionCheck.EXPLICIT); - } - - @Test - public void testGetWithMatchingPermissionsPath() throws Exception { - testGetWithMatchingPermissions(PermissionCheck.PATH); - } - - @Test - public void testGetWithMatchingPermissionsQuery() throws Exception { - testGetWithMatchingPermissions(PermissionCheck.QUERY); - } - - private void testGetWithMatchingPermissions(PermissionCheck permissionCheck) throws Exception { - _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", ImmutableSet.of("testrole"))); + public void testGetWithMatchingPermissions() throws Exception { + _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", IdentityState.ACTIVE, ImmutableSet.of("testrole"))); _permissionDAO.updateForRole("testrole", new PermissionUpdateRequest().permit("city|get|Madrid", "country|get|Spain")); - ClientResponse response = getCountryAndCity(permissionCheck, "Spain", "Madrid", "testkey"); + ClientResponse response = getCountryAndCity("Spain", "Madrid", "testkey"); assertEquals(response.getEntity(String.class), "Welcome to Madrid, Spain"); } @Test - public void testGetWithMatchingWildcardPermissionsExplicit() throws Exception { - testGetWithMatchingWildcardPermissions(PermissionCheck.EXPLICIT); - } - - @Test - public void testGetWithMatchingWildcardPermissionsPath() throws Exception { - testGetWithMatchingWildcardPermissions(PermissionCheck.PATH); - } - - @Test - public void testGetWithMatchingWildcardPermissionsQuery() throws Exception { - testGetWithMatchingWildcardPermissions(PermissionCheck.QUERY); - } - - private void testGetWithMatchingWildcardPermissions(PermissionCheck permissionCheck) throws Exception { - _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", ImmutableSet.of("testrole"))); + public void testGetWithMatchingWildcardPermissions() throws Exception { + _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", IdentityState.ACTIVE, ImmutableSet.of("testrole"))); _permissionDAO.updateForRole("testrole", new PermissionUpdateRequest().permit("city|get|*", "country|*|*")); - ClientResponse response = getCountryAndCity(permissionCheck, "Spain", "Madrid", "testkey"); + ClientResponse response = getCountryAndCity("Spain", "Madrid", "testkey"); assertEquals(response.getEntity(String.class), "Welcome to Madrid, Spain"); } @Test - public void testGetWithNonMatchingWildcardPermissionExplicit() throws Exception { - testGetWithNonMatchingWildcardPermission(PermissionCheck.EXPLICIT); - } - - @Test - public void testGetWithNonMatchingWildcardPermissionPath() throws Exception { - testGetWithNonMatchingWildcardPermission(PermissionCheck.PATH); - } - - @Test - public void testGetWithNonMatchingWildcardPermissionQuery() throws Exception { - testGetWithNonMatchingWildcardPermission(PermissionCheck.QUERY); - } - - private void testGetWithNonMatchingWildcardPermission(PermissionCheck permissionCheck) throws Exception { - _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", ImmutableSet.of("testrole"))); + public void testGetWithNonMatchingWildcardPermission() throws Exception { + _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", IdentityState.ACTIVE, ImmutableSet.of("testrole"))); _permissionDAO.updateForRole("testrole", new PermissionUpdateRequest().permit("city|get|Madrid", "country|*|Portugal")); - ClientResponse response = getCountryAndCity(permissionCheck, "Spain", "Madrid", "testkey"); + ClientResponse response = getCountryAndCity("Spain", "Madrid", "testkey"); assertEquals(response.getStatus(), Response.Status.FORBIDDEN.getStatusCode()); } @Test - public void testGetWithEscapedPermissionExplicit() throws Exception { - testGetWithEscapedPermission(PermissionCheck.EXPLICIT); - } - - @Test - public void testGetWithEscapedPermissionPath() throws Exception { - testGetWithEscapedPermission(PermissionCheck.PATH); - } - - @Test - public void testGetWithEscapedPermissionQuery() throws Exception { - testGetWithEscapedPermission(PermissionCheck.QUERY); - } - - private void testGetWithEscapedPermission(PermissionCheck permissionCheck) throws Exception { - _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", ImmutableSet.of("testrole"))); + public void testGetWithEscapedPermission() throws Exception { + _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", IdentityState.ACTIVE, ImmutableSet.of("testrole"))); _permissionDAO.updateForRole("testrole", new PermissionUpdateRequest().permit("city|get|Pipe\\|Town", "country|get|Star\\*Nation")); - ClientResponse response = getCountryAndCity(permissionCheck, "Star*Nation", "Pipe|Town", "testkey"); + ClientResponse response = getCountryAndCity("Star*Nation", "Pipe|Town", "testkey"); assertEquals(response.getEntity(String.class), "Welcome to Pipe|Town, Star*Nation"); } @Test - public void testAnonymousWithPermissionExplicit() throws Exception { - testAnonymousWithPermission(PermissionCheck.EXPLICIT); - } + public void testGetWithInactiveAPIKey() throws Exception { + // API key has role that can do anything + _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", IdentityState.ACTIVE, ImmutableSet.of("testrole"))); + _permissionDAO.updateForRole("testrole", new PermissionUpdateRequest().permit("*")); - @Test - public void testAnonymousWithPermissionPath() throws Exception { - testAnonymousWithPermission(PermissionCheck.PATH); - } + ClientResponse response = getCountryAndCity("Spain", "Madrid", "testkey"); + assertEquals(response.getStatus(), Response.Status.OK.getStatusCode()); - @Test - public void testAnonymousWithPermissionQuery() throws Exception { - testAnonymousWithPermission(PermissionCheck.QUERY); + // Inactivate the API key and try again + _authIdentityDAO.updateIdentity(new ApiKey("testkey", "id0", IdentityState.INACTIVE, ImmutableSet.of("testrole"))); + response = getCountryAndCity("Spain", "Madrid", "testkey"); + assertEquals(response.getStatus(), Response.Status.FORBIDDEN.getStatusCode()); } - private void testAnonymousWithPermission(PermissionCheck permissionCheck) throws Exception { - _authIdentityDAO.updateIdentity(new ApiKey("anon", "id1", ImmutableSet.of("anonrole"))); + @Test + public void testAnonymousWithPermission() throws Exception { + _authIdentityDAO.updateIdentity(new ApiKey("anon", "id1", IdentityState.ACTIVE, ImmutableSet.of("anonrole"))); _permissionDAO.updateForRole("anonrole", new PermissionUpdateRequest().permit("city|get|Madrid", "country|get|Spain")); - ClientResponse response = getCountryAndCity(permissionCheck, "Spain", "Madrid", null); + ClientResponse response = getCountryAndCity("Spain", "Madrid", null); assertEquals(response.getEntity(String.class), "Welcome to Madrid, Spain"); } @Test - public void testAnonymousWithoutPermissionExplicit() throws Exception { - testAnonymousWithoutPermission(PermissionCheck.EXPLICIT); - } - - @Test - public void testAnonymousWithoutPermissionQuery() throws Exception { - testAnonymousWithoutPermission(PermissionCheck.PATH); - } - - @Test - public void testAnonymousWithoutPermissionPath() throws Exception { - testAnonymousWithoutPermission(PermissionCheck.QUERY); - } - - private void testAnonymousWithoutPermission(PermissionCheck permissionCheck) throws Exception { - ClientResponse response = getCountryAndCity(permissionCheck, "Spain", "Madrid", null); + public void testAnonymousWithoutPermission() throws Exception { + ClientResponse response = getCountryAndCity("Spain", "Madrid", null); assertEquals(response.getStatus(), Response.Status.FORBIDDEN.getStatusCode()); } - private Map _uriFormatMap = ImmutableMap.of( - PermissionCheck.EXPLICIT, "/explicit/country/%s/city/%s", - PermissionCheck.PATH, "/path/country/%s/city/%s", - PermissionCheck.QUERY, "/query/welcome?country=%s&city=%s"); - - private ClientResponse getCountryAndCity(PermissionCheck permissionCheck, String country, String city, String apiKey) + private ClientResponse getCountryAndCity(String country, String city, String apiKey) throws Exception { - String uri = format(_uriFormatMap.get(permissionCheck), URLEncoder.encode(country, "UTF-8"), URLEncoder.encode(city, "UTF-8")); + String uri = _permissionCheck.getUri(country, city); WebResource resource = _resourceTestRule.client().resource(uri); if (apiKey != null) { return resource.header(ApiKeyRequest.AUTHENTICATION_HEADER, apiKey).get(ClientResponse.class); diff --git a/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/apikey/ApiKeyRealmTest.java b/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/apikey/ApiKeyRealmTest.java index a7affdd2b7..b3fafe3b9b 100644 --- a/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/apikey/ApiKeyRealmTest.java +++ b/auth/auth-test/src/test/java/com/bazaarvoice/emodb/auth/apikey/ApiKeyRealmTest.java @@ -2,6 +2,7 @@ import com.bazaarvoice.emodb.auth.identity.AuthIdentityManager; import com.bazaarvoice.emodb.auth.identity.CacheManagingAuthIdentityManager; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.identity.InMemoryAuthIdentityManager; import com.bazaarvoice.emodb.auth.permissions.MatchingPermissionResolver; import com.bazaarvoice.emodb.auth.permissions.PermissionManager; @@ -33,7 +34,6 @@ import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; -import static org.testng.Assert.assertNotEquals; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertNull; import static org.testng.Assert.assertTrue; @@ -51,7 +51,7 @@ public void setup() { CacheRegistry cacheRegistry = new DefaultCacheRegistry(new SimpleLifeCycleRegistry(), new MetricRegistry()); InvalidatableCacheManager _cacheManager = new GuavaCacheManager(cacheRegistry); - InMemoryAuthIdentityManager authIdentityDAO = new InMemoryAuthIdentityManager<>(); + InMemoryAuthIdentityManager authIdentityDAO = new InMemoryAuthIdentityManager<>(ApiKey.class); _authIdentityManager = new CacheManagingAuthIdentityManager<>(authIdentityDAO, _cacheManager); _permissionManager = mock(PermissionManager.class); @@ -216,7 +216,7 @@ public Set answer(InvocationOnMock invocationOnMock) throws Throwabl @Test public void testPermissionCheckByInternalId() { - ApiKey apiKey = new ApiKey("apikey0", "id0", ImmutableList.of("role0")); + ApiKey apiKey = new ApiKey("apikey0", "id0", IdentityState.ACTIVE, ImmutableList.of("role0")); _authIdentityManager.updateIdentity(apiKey); Permission rolePermission = mock(Permission.class); Permission positivePermission = mock(Permission.class); @@ -243,7 +243,7 @@ public void testPermissionCheckByInternalId() { @Test public void testCachedPermissionCheckByInternalId() { - ApiKey apiKey = new ApiKey("apikey0", "id0", ImmutableList.of("role0")); + ApiKey apiKey = new ApiKey("apikey0", "id0", IdentityState.ACTIVE, ImmutableList.of("role0")); _authIdentityManager.updateIdentity(apiKey); Permission rolePermission = mock(Permission.class); Permission positivePermission = mock(Permission.class); diff --git a/docs/_posts/2016-08-31-securityadmin.md b/docs/_posts/2016-08-31-securityadmin.md index 42397a7743..440eab7974 100644 --- a/docs/_posts/2016-08-31-securityadmin.md +++ b/docs/_posts/2016-08-31-securityadmin.md @@ -131,13 +131,15 @@ roles: record_update, databus_standard issued: 11/13/14 12:01 PM ``` -### Delete API key +### Inactivate API key -You can delete an API key with the _delete_ action as in the following example: +An inactive key can no longer be authenticated for access or authorized for any permissions. Effectively it is +deleted, although internally a record of the key exists to prevent re-use. You can inactivate an API key with the +_inactivate_ action as in the following example: ``` -$ curl -XPOST 'localhost:8081/tasks/api-key?action=delete&APIKey=pebbles&key=kp7w6odzin5zki7riqhduadisi7a6wa7cobbfbb379e3z6q5' -API key deleted +$ curl -XPOST 'localhost:8081/tasks/api-key?action=inactivate&APIKey=pebbles&key=kp7w6odzin5zki7riqhduadisi7a6wa7cobbfbb379e3z6q5' +API key inactivated ``` Permissions diff --git a/quality/integration/src/test/java/com/bazaarvoice/emodb/test/ResourceTest.java b/quality/integration/src/test/java/com/bazaarvoice/emodb/test/ResourceTest.java index 88892c36a7..d4f1a7b19f 100644 --- a/quality/integration/src/test/java/com/bazaarvoice/emodb/test/ResourceTest.java +++ b/quality/integration/src/test/java/com/bazaarvoice/emodb/test/ResourceTest.java @@ -38,7 +38,7 @@ protected static ResourceTestRule setupResourceTestRule(List resourceLis } protected static ResourceTestRule setupResourceTestRule(List resourceList, List filters, ApiKey apiKey, ApiKey unauthorizedKey, String typeName) { - InMemoryAuthIdentityManager authIdentityManager = new InMemoryAuthIdentityManager<>(); + InMemoryAuthIdentityManager authIdentityManager = new InMemoryAuthIdentityManager<>(ApiKey.class); authIdentityManager.updateIdentity(apiKey); authIdentityManager.updateIdentity(unauthorizedKey); diff --git a/quality/integration/src/test/java/test/integration/auth/ApiKeyAdminTaskTest.java b/quality/integration/src/test/java/test/integration/auth/ApiKeyAdminTaskTest.java index 9eb029f0ce..e6ce658629 100644 --- a/quality/integration/src/test/java/test/integration/auth/ApiKeyAdminTaskTest.java +++ b/quality/integration/src/test/java/test/integration/auth/ApiKeyAdminTaskTest.java @@ -4,6 +4,7 @@ import com.bazaarvoice.emodb.auth.apikey.ApiKeyRealm; import com.bazaarvoice.emodb.auth.apikey.ApiKeyRequest; import com.bazaarvoice.emodb.auth.apikey.ApiKeySecurityManager; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.identity.InMemoryAuthIdentityManager; import com.bazaarvoice.emodb.auth.permissions.InMemoryPermissionManager; import com.bazaarvoice.emodb.auth.permissions.PermissionUpdateRequest; @@ -28,8 +29,10 @@ import static org.mockito.Mockito.mock; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertNull; +import static org.testng.Assert.assertTrue; public class ApiKeyAdminTaskTest { @@ -39,7 +42,7 @@ public class ApiKeyAdminTaskTest { @BeforeMethod public void setUp() { - _authIdentityManager = new InMemoryAuthIdentityManager<>(); + _authIdentityManager = new InMemoryAuthIdentityManager<>(ApiKey.class); EmoPermissionResolver permissionResolver = new EmoPermissionResolver(mock(DataStore.class), mock(BlobStore.class)); _permissionManager = new InMemoryPermissionManager(permissionResolver); @@ -50,7 +53,7 @@ public void setUp() { _task = new ApiKeyAdminTask(securityManager, mock(TaskRegistry.class), _authIdentityManager, HostAndPort.fromParts("0.0.0.0", 8080), ImmutableSet.of("reservedrole")); - _authIdentityManager.updateIdentity(new ApiKey("test-admin", "id_admin", ImmutableSet.of(DefaultRoles.admin.toString()))); + _authIdentityManager.updateIdentity(new ApiKey("test-admin", "id_admin", IdentityState.ACTIVE, ImmutableSet.of(DefaultRoles.admin.toString()))); } @AfterMethod @@ -83,7 +86,7 @@ public void testCreateNewApiKey() throws Exception { public void testUpdateApiKey() throws Exception { String key = "updateapikeytestkey"; - _authIdentityManager.updateIdentity(new ApiKey(key, "id_update", ImmutableSet.of("role1", "role2", "role3"))); + _authIdentityManager.updateIdentity(new ApiKey(key, "id_update", IdentityState.ACTIVE, ImmutableSet.of("role1", "role2", "role3"))); _task.execute(ImmutableMultimap.builder() .put(ApiKeyRequest.AUTHENTICATION_PARAM, "test-admin") @@ -102,7 +105,7 @@ public void testUpdateApiKey() throws Exception { public void testMigrateApiKey() throws Exception { String key = "migrateapikeytestkey"; - _authIdentityManager.updateIdentity(new ApiKey(key, "id_migrate", ImmutableSet.of("role1", "role2"))); + _authIdentityManager.updateIdentity(new ApiKey(key, "id_migrate", IdentityState.ACTIVE, ImmutableSet.of("role1", "role2"))); assertNotNull(_authIdentityManager.getIdentity(key)); StringWriter output = new StringWriter(); @@ -114,22 +117,45 @@ public void testMigrateApiKey() throws Exception { ApiKey apiKey = _authIdentityManager.getIdentity(newKey); assertNotNull(apiKey); + assertEquals(apiKey.getState(), IdentityState.ACTIVE); assertEquals(apiKey.getRoles(), ImmutableSet.of("role1", "role2")); assertEquals(apiKey.getInternalId(), "id_migrate"); - assertNull(_authIdentityManager.getIdentity(key)); + assertEquals(_authIdentityManager.getIdentity(key).getState(), IdentityState.MIGRATED); + } + + @Test + public void testInactivateApiKey() throws Exception { + String key = "inactivateapikeytestkey"; + + _authIdentityManager.updateIdentity(new ApiKey(key, "id_inactive", IdentityState.ACTIVE, ImmutableSet.of("role1", "role2"))); + assertEquals(_authIdentityManager.getIdentity(key).getState(), IdentityState.ACTIVE); + + _task.execute(ImmutableMultimap.of( + ApiKeyRequest.AUTHENTICATION_PARAM, "test-admin", + "action", "inactivate", "key", key), mock(PrintWriter.class)); + assertEquals(_authIdentityManager.getIdentity(key).getState(), IdentityState.INACTIVE); } @Test public void testDeleteApiKey() throws Exception { String key = "deleteapikeytestkey"; - _authIdentityManager.updateIdentity(new ApiKey(key, "id_delete", ImmutableSet.of("role1", "role2"))); - assertNotNull(_authIdentityManager.getIdentity(key)); + _authIdentityManager.updateIdentity(new ApiKey(key, "id_delete", IdentityState.ACTIVE, ImmutableSet.of("role1", "role2"))); + assertEquals(_authIdentityManager.getIdentity(key).getState(), IdentityState.ACTIVE); + // Attempt to delete the key without the confirmation parameter _task.execute(ImmutableMultimap.of( ApiKeyRequest.AUTHENTICATION_PARAM, "test-admin", "action", "delete", "key", key), mock(PrintWriter.class)); + // The key should be unchanged + assertEquals(_authIdentityManager.getIdentity(key).getState(), IdentityState.ACTIVE); + + // Delete the key again, this time with the necessary confirmation parameter + _task.execute(ImmutableMultimap.of( + ApiKeyRequest.AUTHENTICATION_PARAM, "test-admin", + "action", "delete", "key", key, "confirm", "true"), mock(PrintWriter.class)); assertNull(_authIdentityManager.getIdentity(key)); + assertNull(_authIdentityManager.getInternalIdentity("id_delete")); } @Test diff --git a/quality/integration/src/test/java/test/integration/auth/RoleAdminTaskTest.java b/quality/integration/src/test/java/test/integration/auth/RoleAdminTaskTest.java index 38a2e1f613..157430ff8c 100644 --- a/quality/integration/src/test/java/test/integration/auth/RoleAdminTaskTest.java +++ b/quality/integration/src/test/java/test/integration/auth/RoleAdminTaskTest.java @@ -4,6 +4,7 @@ import com.bazaarvoice.emodb.auth.apikey.ApiKeyRealm; import com.bazaarvoice.emodb.auth.apikey.ApiKeyRequest; import com.bazaarvoice.emodb.auth.apikey.ApiKeySecurityManager; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.identity.InMemoryAuthIdentityManager; import com.bazaarvoice.emodb.auth.permissions.InMemoryPermissionManager; import com.bazaarvoice.emodb.auth.permissions.PermissionUpdateRequest; @@ -46,7 +47,7 @@ public class RoleAdminTaskTest { @BeforeMethod public void setUp() { - _authIdentityManager = new InMemoryAuthIdentityManager<>(); + _authIdentityManager = new InMemoryAuthIdentityManager<>(ApiKey.class); EmoPermissionResolver permissionResolver = new EmoPermissionResolver(mock(DataStore.class), mock(BlobStore.class)); _permissionManager = new InMemoryPermissionManager(permissionResolver); @@ -56,7 +57,7 @@ public void setUp() { null)); _task = new RoleAdminTask(securityManager, _permissionManager, mock(TaskRegistry.class)); - _authIdentityManager.updateIdentity(new ApiKey("test-admin", "id_admin", ImmutableSet.of(DefaultRoles.admin.toString()))); + _authIdentityManager.updateIdentity(new ApiKey("test-admin", "id_admin", IdentityState.ACTIVE, ImmutableSet.of(DefaultRoles.admin.toString()))); } @AfterMethod diff --git a/quality/integration/src/test/java/test/integration/auth/TableAuthIdentityManagerTest.java b/quality/integration/src/test/java/test/integration/auth/TableAuthIdentityManagerTest.java index 5872ac4cb4..e2354e47c6 100644 --- a/quality/integration/src/test/java/test/integration/auth/TableAuthIdentityManagerTest.java +++ b/quality/integration/src/test/java/test/integration/auth/TableAuthIdentityManagerTest.java @@ -1,6 +1,8 @@ package test.integration.auth; import com.bazaarvoice.emodb.auth.apikey.ApiKey; +import com.bazaarvoice.emodb.auth.identity.IdentityState; +import com.bazaarvoice.emodb.auth.identity.InternalIdentity; import com.bazaarvoice.emodb.auth.identity.TableAuthIdentityManager; import com.bazaarvoice.emodb.sor.api.AuditBuilder; import com.bazaarvoice.emodb.sor.api.DataStore; @@ -16,11 +18,12 @@ 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; +import static org.testng.Assert.assertNull; +import static org.testng.Assert.assertTrue; public class TableAuthIdentityManagerTest { @@ -38,7 +41,7 @@ public void testRebuildInternalIdIndex() { TableAuthIdentityManager 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 apiKey = new ApiKey("testkey", "id0", IdentityState.ACTIVE, ImmutableSet.of("role1", "role2")); apiKey.setOwner("testowner"); tableAuthIdentityManager.updateIdentity(apiKey); @@ -59,8 +62,11 @@ public void testRebuildInternalIdIndex() { new AuditBuilder().setComment("test delete").build()); // Verify that a lookup by internal ID works - Set roles = tableAuthIdentityManager.getRolesByInternalId("id0"); - assertEquals(roles, ImmutableSet.of("role1", "role2")); + InternalIdentity internalIdentity = tableAuthIdentityManager.getInternalIdentity("id0"); + assertEquals(internalIdentity.getInternalId(), "id0"); + assertEquals(internalIdentity.getState(), IdentityState.ACTIVE); + assertEquals(internalIdentity.getOwner(), "testowner"); + assertEquals(internalIdentity.getRoles(), ImmutableSet.of("role1", "role2")); // Verify that the index record is re-created indexMap = dataStore.get("__auth:internal_ids", "id0"); @@ -81,7 +87,8 @@ public void testGrandfatheredInInternalId() { 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. + // Write out a record which mimics the pre-internal-id format. Notably missing are the "internalId" + // and "state" attributes. Map oldIdentityMap = ImmutableMap.builder() .put("maskedId", "aaaa****************************************llll") .put("owner", "someone") @@ -102,8 +109,11 @@ public void testGrandfatheredInInternalId() { assertEquals(apiKey.getRoles(), ImmutableList.of("role1", "role2")); // Verify that a lookup by internal ID works - Set roles = tableAuthIdentityManager.getRolesByInternalId(hash); - assertEquals(roles, ImmutableSet.of("role1", "role2")); + InternalIdentity internalIdentity = tableAuthIdentityManager.getInternalIdentity(hash); + assertEquals(internalIdentity.getInternalId(), hash); + assertEquals(internalIdentity.getState(), IdentityState.ACTIVE); + assertEquals(internalIdentity.getOwner(), "someone"); + assertEquals(internalIdentity.getRoles(), ImmutableSet.of("role1", "role2")); // Verify that the index record was created with the hashed ID as the internal ID Map indexMap = dataStore.get("__auth:internal_ids", hash); @@ -111,7 +121,32 @@ public void testGrandfatheredInInternalId() { 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")); + internalIdentity = tableAuthIdentityManager.getInternalIdentity(hash); + assertEquals(internalIdentity.getInternalId(), hash); + } + + @Test + public void testDeleteIdentityUnsafe() throws Exception { + DataStore dataStore = new InMemoryDataStore(new MetricRegistry()); + TableAuthIdentityManager tableAuthIdentityManager = new TableAuthIdentityManager<>( + ApiKey.class, dataStore, "__auth:keys", "__auth:internal_ids", "app_global:sys", Hashing.sha256()); + + ApiKey apiKey = new ApiKey("testkey", "id0", IdentityState.ACTIVE, ImmutableSet.of("role1", "role2")); + apiKey.setOwner("testowner"); + tableAuthIdentityManager.updateIdentity(apiKey); + + String keyTableId = Hashing.sha256().hashUnencodedChars("testkey").toString(); + + assertFalse(Intrinsic.isDeleted(dataStore.get("__auth:keys", keyTableId))); + assertFalse(Intrinsic.isDeleted(dataStore.get("__auth:internal_ids", "id0"))); + assertNotNull(tableAuthIdentityManager.getIdentity("testkey")); + assertNotNull(tableAuthIdentityManager.getInternalIdentity("id0")); + + tableAuthIdentityManager.deleteIdentityUnsafe("testkey"); + + assertTrue(Intrinsic.isDeleted(dataStore.get("__auth:keys", keyTableId))); + assertTrue(Intrinsic.isDeleted(dataStore.get("__auth:internal_ids", "id0"))); + assertNull(tableAuthIdentityManager.getIdentity("testkey")); + assertNull(tableAuthIdentityManager.getInternalIdentity("id0")); } } diff --git a/quality/integration/src/test/java/test/integration/blob/BlobStoreJerseyTest.java b/quality/integration/src/test/java/test/integration/blob/BlobStoreJerseyTest.java index 0b7986e496..17d14b5248 100644 --- a/quality/integration/src/test/java/test/integration/blob/BlobStoreJerseyTest.java +++ b/quality/integration/src/test/java/test/integration/blob/BlobStoreJerseyTest.java @@ -1,6 +1,7 @@ package test.integration.blob; import com.bazaarvoice.emodb.auth.apikey.ApiKey; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.identity.InMemoryAuthIdentityManager; import com.bazaarvoice.emodb.auth.permissions.InMemoryPermissionManager; import com.bazaarvoice.emodb.auth.permissions.PermissionUpdateRequest; @@ -102,11 +103,11 @@ public class BlobStoreJerseyTest extends ResourceTest { public ResourceTestRule _resourceTestRule = setupResourceTestRule(); private ResourceTestRule setupResourceTestRule() { - final InMemoryAuthIdentityManager authIdentityManager = new InMemoryAuthIdentityManager<>(); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_BLOB, "id0", ImmutableSet.of("blob-role"))); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_UNAUTHORIZED, "id1", ImmutableSet.of("unauthorized-role"))); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_BLOB_A, "id2", ImmutableSet.of("blob-role-a"))); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_BLOB_B, "id3", ImmutableSet.of("blob-role-b"))); + final InMemoryAuthIdentityManager authIdentityManager = new InMemoryAuthIdentityManager<>(ApiKey.class); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_BLOB, "id0", IdentityState.ACTIVE, ImmutableSet.of("blob-role"))); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_UNAUTHORIZED, "id1", IdentityState.ACTIVE, ImmutableSet.of("unauthorized-role"))); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_BLOB_A, "id2", IdentityState.ACTIVE, ImmutableSet.of("blob-role-a"))); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_BLOB_B, "id3", IdentityState.ACTIVE, ImmutableSet.of("blob-role-b"))); final EmoPermissionResolver permissionResolver = new EmoPermissionResolver(mock(DataStore.class), _server); final InMemoryPermissionManager permissionManager = new InMemoryPermissionManager(permissionResolver); diff --git a/quality/integration/src/test/java/test/integration/databus/DatabusJerseyTest.java b/quality/integration/src/test/java/test/integration/databus/DatabusJerseyTest.java index 62d933a619..d0738ff4ce 100644 --- a/quality/integration/src/test/java/test/integration/databus/DatabusJerseyTest.java +++ b/quality/integration/src/test/java/test/integration/databus/DatabusJerseyTest.java @@ -2,6 +2,7 @@ import com.bazaarvoice.emodb.auth.apikey.ApiKey; import com.bazaarvoice.emodb.auth.apikey.ApiKeyRequest; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.jersey.Subject; import com.bazaarvoice.emodb.common.api.ServiceUnavailableException; import com.bazaarvoice.emodb.common.api.UnauthorizedException; @@ -111,8 +112,8 @@ public class DatabusJerseyTest extends ResourceTest { @Rule public ResourceTestRule _resourceTestRule = setupResourceTestRule( Collections.singletonList(new DatabusResource1(_local, _client, mock(DatabusEventStore.class), new DatabusResourcePoller(new MetricRegistry()))), - new ApiKey(APIKEY_DATABUS, INTERNAL_ID_DATABUS, ImmutableSet.of("databus-role")), - new ApiKey(APIKEY_UNAUTHORIZED, INTERNAL_ID_UNAUTHORIZED, ImmutableSet.of("unauthorized-role")), + new ApiKey(APIKEY_DATABUS, INTERNAL_ID_DATABUS, IdentityState.ACTIVE, ImmutableSet.of("databus-role")), + new ApiKey(APIKEY_UNAUTHORIZED, INTERNAL_ID_UNAUTHORIZED, IdentityState.ACTIVE, ImmutableSet.of("unauthorized-role")), "databus"); @After diff --git a/quality/integration/src/test/java/test/integration/databus/ReplicationJerseyTest.java b/quality/integration/src/test/java/test/integration/databus/ReplicationJerseyTest.java index 87b7ff018d..7f30214130 100644 --- a/quality/integration/src/test/java/test/integration/databus/ReplicationJerseyTest.java +++ b/quality/integration/src/test/java/test/integration/databus/ReplicationJerseyTest.java @@ -1,6 +1,7 @@ package test.integration.databus; import com.bazaarvoice.emodb.auth.apikey.ApiKey; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.identity.InMemoryAuthIdentityManager; import com.bazaarvoice.emodb.auth.permissions.InMemoryPermissionManager; import com.bazaarvoice.emodb.auth.permissions.PermissionUpdateRequest; @@ -38,11 +39,11 @@ public class ReplicationJerseyTest extends ResourceTest { @Rule public ResourceTestRule _resourceTestRule = setupReplicationResourceTestRule(ImmutableList.of(new ReplicationResource1(_server)), - new ApiKey(APIKEY_REPLICATION, "repl", ImmutableSet.of("replication-role")), - new ApiKey(APIKEY_UNAUTHORIZED, "unauth", ImmutableSet.of("unauthorized-role"))); + new ApiKey(APIKEY_REPLICATION, "repl", IdentityState.ACTIVE, ImmutableSet.of("replication-role")), + new ApiKey(APIKEY_UNAUTHORIZED, "unauth", IdentityState.ACTIVE, ImmutableSet.of("unauthorized-role"))); protected static ResourceTestRule setupReplicationResourceTestRule(List resourceList, ApiKey apiKey, ApiKey unauthorizedKey) { - InMemoryAuthIdentityManager authIdentityManager = new InMemoryAuthIdentityManager<>(); + InMemoryAuthIdentityManager authIdentityManager = new InMemoryAuthIdentityManager<>(ApiKey.class); authIdentityManager.updateIdentity(apiKey); authIdentityManager.updateIdentity(unauthorizedKey); diff --git a/quality/integration/src/test/java/test/integration/exceptions/ExceptionMapperJerseyTest.java b/quality/integration/src/test/java/test/integration/exceptions/ExceptionMapperJerseyTest.java index e1b28f3931..6be716451f 100644 --- a/quality/integration/src/test/java/test/integration/exceptions/ExceptionMapperJerseyTest.java +++ b/quality/integration/src/test/java/test/integration/exceptions/ExceptionMapperJerseyTest.java @@ -1,6 +1,7 @@ package test.integration.exceptions; import com.bazaarvoice.emodb.auth.apikey.ApiKey; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.blob.api.BlobNotFoundException; import com.bazaarvoice.emodb.blob.api.RangeNotSatisfiableException; import com.bazaarvoice.emodb.common.json.JsonStreamProcessingException; @@ -39,8 +40,8 @@ public class ExceptionMapperJerseyTest extends ResourceTest { @Rule public ResourceTestRule _resourceTestRule = setupResourceTestRule(Collections.singletonList(new ExceptionResource()), - new ApiKey("unused", "id0", ImmutableSet.of()), - new ApiKey("also-unused", "id1", ImmutableSet.of()), + new ApiKey("unused", "id0", IdentityState.ACTIVE, ImmutableSet.of()), + new ApiKey("also-unused", "id1", IdentityState.ACTIVE, ImmutableSet.of()), "exception"); @Test diff --git a/quality/integration/src/test/java/test/integration/queue/DedupQueueJerseyTest.java b/quality/integration/src/test/java/test/integration/queue/DedupQueueJerseyTest.java index 75048211c5..5525ef751f 100644 --- a/quality/integration/src/test/java/test/integration/queue/DedupQueueJerseyTest.java +++ b/quality/integration/src/test/java/test/integration/queue/DedupQueueJerseyTest.java @@ -1,6 +1,7 @@ package test.integration.queue; import com.bazaarvoice.emodb.auth.apikey.ApiKey; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.common.api.ServiceUnavailableException; import com.bazaarvoice.emodb.common.api.UnauthorizedException; import com.bazaarvoice.emodb.common.jersey.dropwizard.JerseyEmoClient; @@ -56,8 +57,8 @@ public class DedupQueueJerseyTest extends ResourceTest { @Rule public ResourceTestRule _resourceTestRule = setupResourceTestRule(Collections.singletonList(new DedupQueueResource1(_server, DedupQueueServiceAuthenticator.proxied(_proxy))), - new ApiKey(APIKEY_QUEUE, "queue", ImmutableSet.of("queue-role")), - new ApiKey(APIKEY_UNAUTHORIZED, "unauth", ImmutableSet.of("unauthorized-role")), + new ApiKey(APIKEY_QUEUE, "queue", IdentityState.ACTIVE, ImmutableSet.of("queue-role")), + new ApiKey(APIKEY_UNAUTHORIZED, "unauth", IdentityState.ACTIVE, ImmutableSet.of("unauthorized-role")), "queue"); @After diff --git a/quality/integration/src/test/java/test/integration/queue/QueueJerseyTest.java b/quality/integration/src/test/java/test/integration/queue/QueueJerseyTest.java index f6db7a1b51..9566d8ad87 100644 --- a/quality/integration/src/test/java/test/integration/queue/QueueJerseyTest.java +++ b/quality/integration/src/test/java/test/integration/queue/QueueJerseyTest.java @@ -1,6 +1,7 @@ package test.integration.queue; import com.bazaarvoice.emodb.auth.apikey.ApiKey; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.client.EmoClientException; import com.bazaarvoice.emodb.common.api.ServiceUnavailableException; import com.bazaarvoice.emodb.common.api.UnauthorizedException; @@ -57,8 +58,8 @@ public class QueueJerseyTest extends ResourceTest { @Rule public ResourceTestRule _resourceTestRule = setupResourceTestRule(Collections.singletonList(new QueueResource1(_server, QueueServiceAuthenticator.proxied(_proxy))), - new ApiKey(APIKEY_QUEUE, "queue", ImmutableSet.of("queue-role")), - new ApiKey(APIKEY_UNAUTHORIZED, "unauth", ImmutableSet.of("unauthorized-role")), + new ApiKey(APIKEY_QUEUE, "queue", IdentityState.ACTIVE, ImmutableSet.of("queue-role")), + new ApiKey(APIKEY_UNAUTHORIZED, "unauth", IdentityState.ACTIVE, ImmutableSet.of("unauthorized-role")), "queue"); @After diff --git a/quality/integration/src/test/java/test/integration/sor/DataStoreJerseyTest.java b/quality/integration/src/test/java/test/integration/sor/DataStoreJerseyTest.java index 69ed64cfd5..a471ec83f0 100644 --- a/quality/integration/src/test/java/test/integration/sor/DataStoreJerseyTest.java +++ b/quality/integration/src/test/java/test/integration/sor/DataStoreJerseyTest.java @@ -3,6 +3,7 @@ import com.bazaarvoice.emodb.auth.InvalidCredentialException; import com.bazaarvoice.emodb.auth.apikey.ApiKey; import com.bazaarvoice.emodb.auth.apikey.ApiKeyRequest; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.identity.InMemoryAuthIdentityManager; import com.bazaarvoice.emodb.auth.permissions.InMemoryPermissionManager; import com.bazaarvoice.emodb.auth.permissions.PermissionUpdateRequest; @@ -109,6 +110,7 @@ public class DataStoreJerseyTest extends ResourceTest { private static final String APIKEY_REVIEWS_ONLY = "reviews-only-key"; private static final String APIKEY_STANDARD = "standard-key"; private static final String APIKEY_STANDARD_UPDATE = "standard-update"; + private static final String APIKEY_INACTIVE = "inactive"; private DataStore _server = mock(DataStore.class); private DataCenters _dataCenters = mock(DataCenters.class); @@ -117,14 +119,15 @@ public class DataStoreJerseyTest extends ResourceTest { public ResourceTestRule _resourceTestRule = setupDataStoreResourceTestRule(); private ResourceTestRule setupDataStoreResourceTestRule() { - InMemoryAuthIdentityManager authIdentityManager = new InMemoryAuthIdentityManager<>(); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_TABLE, "id0", ImmutableSet.of("table-role"))); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_READ_TABLES_A, "id1", ImmutableSet.of("tables-a-role"))); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_READ_TABLES_B, "id2", ImmutableSet.of("tables-b-role"))); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_FACADE, "id3", ImmutableSet.of("facade-role"))); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_REVIEWS_ONLY, "id4", ImmutableSet.of("reviews-only-role"))); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_STANDARD, "id5", ImmutableSet.of("standard"))); - authIdentityManager.updateIdentity(new ApiKey(APIKEY_STANDARD_UPDATE, "id5", ImmutableSet.of("update-with-events"))); + InMemoryAuthIdentityManager authIdentityManager = new InMemoryAuthIdentityManager<>(ApiKey.class); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_TABLE, "id0", IdentityState.ACTIVE, ImmutableSet.of("table-role"))); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_READ_TABLES_A, "id1", IdentityState.ACTIVE, ImmutableSet.of("tables-a-role"))); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_READ_TABLES_B, "id2", IdentityState.ACTIVE, ImmutableSet.of("tables-b-role"))); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_FACADE, "id3", IdentityState.ACTIVE, ImmutableSet.of("facade-role"))); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_REVIEWS_ONLY, "id4", IdentityState.ACTIVE, ImmutableSet.of("reviews-only-role"))); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_STANDARD, "id5", IdentityState.ACTIVE, ImmutableSet.of("standard"))); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_STANDARD_UPDATE, "id5", IdentityState.ACTIVE, ImmutableSet.of("update-with-events"))); + authIdentityManager.updateIdentity(new ApiKey(APIKEY_INACTIVE, "id6", IdentityState.INACTIVE, ImmutableSet.of("table-role"))); EmoPermissionResolver permissionResolver = new EmoPermissionResolver(_server, mock(BlobStore.class)); InMemoryPermissionManager permissionManager = new InMemoryPermissionManager(permissionResolver); @@ -1337,4 +1340,10 @@ public void testClientWithNullApiKey() { public void testClientWithEmptyApiKey() { sorClient(""); } + + @Test (expected = UnauthorizedException.class) + public void testInactiveApiKey() { + // APIKEY_INACTIVE has permission to list tables, but since it is inactive this request should be denied + sorClient(APIKEY_INACTIVE).listTables(null, 10); + } } diff --git a/quality/integration/src/test/java/test/integration/throttle/AdHocThrottleTest.java b/quality/integration/src/test/java/test/integration/throttle/AdHocThrottleTest.java index 6c3ff2676a..83bcbedb6d 100644 --- a/quality/integration/src/test/java/test/integration/throttle/AdHocThrottleTest.java +++ b/quality/integration/src/test/java/test/integration/throttle/AdHocThrottleTest.java @@ -1,6 +1,7 @@ package test.integration.throttle; import com.bazaarvoice.emodb.auth.apikey.ApiKey; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.identity.InMemoryAuthIdentityManager; import com.bazaarvoice.emodb.auth.permissions.InMemoryPermissionManager; import com.bazaarvoice.emodb.auth.permissions.PermissionUpdateRequest; @@ -99,8 +100,8 @@ public class AdHocThrottleTest extends ResourceTest { public ResourceTestRule _resourceTestRule = setupDataStoreResourceTestRule(); private ResourceTestRule setupDataStoreResourceTestRule() { - InMemoryAuthIdentityManager authIdentityManager = new InMemoryAuthIdentityManager<>(); - authIdentityManager.updateIdentity(new ApiKey(API_KEY, "id", ImmutableList.of("all-sor-role"))); + InMemoryAuthIdentityManager authIdentityManager = new InMemoryAuthIdentityManager<>(ApiKey.class); + authIdentityManager.updateIdentity(new ApiKey(API_KEY, "id", IdentityState.ACTIVE, ImmutableList.of("all-sor-role"))); EmoPermissionResolver permissionResolver = new EmoPermissionResolver(_dataStore, mock(BlobStore.class)); InMemoryPermissionManager permissionManager = new InMemoryPermissionManager(permissionResolver); permissionManager.updateForRole( diff --git a/web/src/main/java/com/bazaarvoice/emodb/web/auth/ApiKeyAdminTask.java b/web/src/main/java/com/bazaarvoice/emodb/web/auth/ApiKeyAdminTask.java index e3790ac5f8..8299e11486 100644 --- a/web/src/main/java/com/bazaarvoice/emodb/web/auth/ApiKeyAdminTask.java +++ b/web/src/main/java/com/bazaarvoice/emodb/web/auth/ApiKeyAdminTask.java @@ -4,13 +4,12 @@ import com.bazaarvoice.emodb.auth.apikey.ApiKeyAuthenticationToken; import com.bazaarvoice.emodb.auth.apikey.ApiKeyRequest; import com.bazaarvoice.emodb.auth.identity.AuthIdentityManager; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.common.dropwizard.guice.SelfHostAndPort; import com.bazaarvoice.emodb.common.dropwizard.task.TaskRegistry; import com.bazaarvoice.emodb.common.json.ISO8601DateFormat; import com.bazaarvoice.emodb.common.uuid.TimeUUIDs; import com.google.common.base.Joiner; -import com.google.common.base.Optional; -import com.google.common.base.Predicates; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -81,20 +80,20 @@ * Migrate API key * =============== * - * The following example copies all metadata and associated roles from "sample-key" to a new key then - * deletes "sample-key". + * The following example copies all metadata and associated roles from "sample-key" to a new key, removing + * the ability for "sample-key" to be authorized or authenticated. * * * $ curl -XPOST "localhost:8081/tasks/api-key?action=migrate&APIKey=admin-key&key=sample-key" * * - * Delete API key + * Inactivate API key * ============== * - * The following example deletes API key "sample-key". + * The following example inactivates API key "sample-key". * * - * $ curl -XPOST "localhost:8081/tasks/api-key?action=delete&APIKey=admin-key&key=sample-key" + * $ curl -XPOST "localhost:8081/tasks/api-key?action=inactivate&APIKey=admin-key&key=sample-key" * */ public class ApiKeyAdminTask extends Task { @@ -106,13 +105,14 @@ private enum Action { VIEW, UPDATE, MIGRATE, + INACTIVATE, DELETE } private final SecurityManager _securityManager; private final AuthIdentityManager _authIdentityManager; - private final HostAndPort _hostAndPort; private final Set _reservedRoles; + private final SecureRandom _secureRandom; @Inject public ApiKeyAdminTask(SecurityManager securityManager, @@ -124,9 +124,15 @@ public ApiKeyAdminTask(SecurityManager securityManager, _securityManager = securityManager; _authIdentityManager = authIdentityManager; - _hostAndPort = selfHostAndPort; _reservedRoles = reservedRoles; + // Create a randomizer that makes the odds of creating the same API key globally effectively zero + _secureRandom = new SecureRandom(); + _secureRandom.setSeed(System.currentTimeMillis()); + _secureRandom.setSeed(Thread.currentThread().getId()); + _secureRandom.setSeed(selfHostAndPort.getHostText().getBytes()); + _secureRandom.setSeed(selfHostAndPort.getPort()); + taskRegistry.addTask(this); } @@ -158,6 +164,9 @@ public void execute(ImmutableMultimap parameters, PrintWriter ou case MIGRATE: migrateApiKey(parameters, output); break; + case INACTIVATE: + inactivateApiKey(parameters, output); + break; case DELETE: deleteApiKey(parameters, output); break; @@ -179,80 +188,80 @@ private String createUniqueInternalId() { return BaseEncoding.base32().omitPadding().encode(b); } + /** + * When creating or modifying a key the caller can provide an explicit key. This isn't common and should be + * restricted to integration tests where stable keys are desirable. In production it is better + * to let the system create random keys. + */ + private String getUniqueOrProvidedKey(ImmutableMultimap parameters, String explicitKeyParam, PrintWriter output) { + String key = Iterables.getOnlyElement(parameters.get(explicitKeyParam), null); + + if (key != null) { + if (!isProvidedApiKeyValid(key)) { + output.println("Error: Provided key is not valid"); + return null; + } + if (isApiKeyInUse(key)) { + output.println("Error: Provided key exists"); + return null; + } + } else { + key = createUniqueApiKey(); + } + + return key; + } + private void createApiKey(ImmutableMultimap parameters, PrintWriter output) throws Exception { String owner = getValueFromParams("owner", parameters); Set roles = ImmutableSet.copyOf(parameters.get("role")); String description = Iterables.getFirst(parameters.get("description"), null); - // If the caller provided a specific key then only use that one. This isn't common and should be - // restricted to integration tests where stable keys are desirable. In production it is better - // to let the system create random keys. - Optional providedKey = Iterables.tryFind(parameters.get("key"), Predicates.alwaysTrue()); - checkArgument(Sets.intersection(roles, _reservedRoles).isEmpty(), "Cannot assign reserved role"); // Generate a unique internal ID for this new key String internalId = createUniqueInternalId(); - String key; - if (providedKey.isPresent()) { - key = providedKey.get(); - if (!isProvidedApiKeyValid(key)) { - output.println("Error: Provided key is not valid"); - return; - } - if (!createApiKeyIfAvailable(key, internalId, owner, roles, description)) { - output.println("Error: Provided key exists"); - return; - } - } else { - key = createRandomApiKey(internalId, owner, roles, description); + String key = getUniqueOrProvidedKey(parameters, "key", output); + if (key == null) { + return; } + createApiKey(key, internalId, owner, roles, description); + output.println("API key: " + key); output.println("\nWarning: This is your only chance to see this key. Save it somewhere now."); } - private boolean createApiKeyIfAvailable(String key, String internalId, String owner, Set roles, String description) { - boolean exists = _authIdentityManager.getIdentity(key) != null; - - if (exists) { - return false; - } - - ApiKey apiKey = new ApiKey(key, internalId, roles); + private void createApiKey(String key, String internalId, String owner, Set roles, String description) { + ApiKey apiKey = new ApiKey(key, internalId, IdentityState.ACTIVE, roles); apiKey.setOwner(owner); apiKey.setDescription(description); apiKey.setIssued(new Date()); _authIdentityManager.updateIdentity(apiKey); - - return true; } - private String createRandomApiKey(String internalId, String owner, Set roles, String description) { - // Since API keys are stored hashed we create them in a loop to ensure we don't grab one that is already picked + private boolean isApiKeyInUse(String key) { + return _authIdentityManager.getIdentity(key) != null; + } - String key = null; - boolean apiKeyCreated = false; + private String createUniqueApiKey() { + int attempt = 0; - while (!apiKeyCreated) { - key = generateRandomApiKey(); - apiKeyCreated = createApiKeyIfAvailable(key, internalId, owner, roles, description); + while (attempt++ < 10) { + String key = generateRandomApiKey(); + if (!isApiKeyInUse(key)) { + return key; + } } - return key; + // Instead of trying indefinitely, raise an exception if no unique API key was generated after 10 attempts + throw new RuntimeException("Failed to generate unique API key after 10 attempts"); } - private String generateRandomApiKey() { - // Randomize the API key such that it is practically assured that no two call will create the same API key - // at the same time. - SecureRandom random = new SecureRandom(); - random.setSeed(System.currentTimeMillis()); - random.setSeed(Thread.currentThread().getId()); - random.setSeed(_hostAndPort.getHostText().getBytes()); - random.setSeed(_hostAndPort.getPort()); + private synchronized String generateRandomApiKey() { // Use base64 encoding but keep the keys alphanumeric (we could use base64URL() to make them at least URL-safe // but pure alphanumeric keeps validation simple). @@ -260,7 +269,7 @@ private String generateRandomApiKey() { byte[] rawKey = new byte[36]; String key = ""; do { - random.nextBytes(rawKey); + _secureRandom.nextBytes(rawKey); String chars = BaseEncoding.base64().omitPadding().encode(rawKey).toLowerCase(); // Eliminate all '+' an '/' characters chars = chars.replaceAll("\\+|/", ""); @@ -284,6 +293,7 @@ private void viewApiKey(ImmutableMultimap parameters, PrintWrite } else { output.println("owner: " + apiKey.getOwner()); output.println("description: " + apiKey.getDescription()); + output.println("state: " + apiKey.getState()); output.println("roles: " + Joiner.on(", ").join(apiKey.getRoles())); output.println("issued: " + ISO8601DateFormat.getInstance().format(apiKey.getIssued())); } @@ -306,7 +316,7 @@ private void updateApiKey(ImmutableMultimap parameters, PrintWri roles.removeAll(removeRoles); if (!roles.equals(apiKey.getRoles())) { - ApiKey updatedKey = new ApiKey(key, apiKey.getInternalId(), roles); + ApiKey updatedKey = new ApiKey(key, apiKey.getInternalId(), apiKey.getState(), roles); updatedKey.setOwner(apiKey.getOwner()); updatedKey.setDescription(apiKey.getDescription()); updatedKey.setIssued(new Date()); @@ -319,23 +329,48 @@ private void updateApiKey(ImmutableMultimap parameters, PrintWri private void migrateApiKey(ImmutableMultimap parameters, PrintWriter output) { String key = getValueFromParams("key", parameters); - ApiKey apiKey = _authIdentityManager.getIdentity(key); - checkArgument(apiKey != null, "Unknown API key"); + checkArgument(isApiKeyInUse(key), "Unknown API key"); - // Create a new key with the same information as the existing one - //noinspection ConstantConditions - String newKey = createRandomApiKey(apiKey.getInternalId(), apiKey.getOwner(), apiKey.getRoles(), apiKey.getDescription()); - // Delete the existing key - _authIdentityManager.deleteIdentity(key); + String newKey = getUniqueOrProvidedKey(parameters, "newKey", output); + if (newKey == null) { + return; + } + + _authIdentityManager.migrateIdentity(key, newKey); output.println("Migrated API key: " + newKey); output.println("\nWarning: This is your only chance to see this key. Save it somewhere now."); } + private void inactivateApiKey(ImmutableMultimap parameters, PrintWriter output) { + String key = getValueFromParams("key", parameters); + ApiKey apiKey = _authIdentityManager.getIdentity(key); + checkArgument(apiKey != null, "Unknown API key"); + checkArgument(apiKey.getState().isActive(), "Cannot inactivate API key in state %s", apiKey.getState()); + apiKey.setState(IdentityState.INACTIVE); + _authIdentityManager.updateIdentity(apiKey); + output.println("API key inactivated"); + } + private void deleteApiKey(ImmutableMultimap parameters, PrintWriter output) { String key = getValueFromParams("key", parameters); - _authIdentityManager.deleteIdentity(key); - output.println("API key deleted"); + + // Normally it is not safe to delete an API key. Doing so can open vectors for recreating the key or having a + // new key whose hash collides with the deleted key. However, for unit testing purposes there is a need to + // actually delete the key from the system. Historically the "delete" action was used for what is now + // "inactivate". To prevent accidental deletion and protect against users using the older "delete" action an + // extra confirmation parameter is required. + + boolean confirmed = parameters.get("confirm").stream().anyMatch("true"::equalsIgnoreCase); + + if (!confirmed) { + output.println("Deleting an API key is a potentially unsafe operation that should only be performed in limited circumstances."); + output.println("If the intent is to make this API key unusable call this task again with the 'inactivate' action."); + output.println("To confirm permanently deleting this API key call this task again with a 'confirm=true' parameter"); + } else { + _authIdentityManager.deleteIdentityUnsafe(key); + output.println("API key deleted"); + } } private String getValueFromParams(String value, ImmutableMultimap parameters) { diff --git a/web/src/main/java/com/bazaarvoice/emodb/web/auth/SecurityModule.java b/web/src/main/java/com/bazaarvoice/emodb/web/auth/SecurityModule.java index fbbe4b84cd..2a249ebc19 100644 --- a/web/src/main/java/com/bazaarvoice/emodb/web/auth/SecurityModule.java +++ b/web/src/main/java/com/bazaarvoice/emodb/web/auth/SecurityModule.java @@ -9,6 +9,7 @@ import com.bazaarvoice.emodb.auth.identity.AuthIdentityManager; import com.bazaarvoice.emodb.auth.identity.CacheManagingAuthIdentityManager; import com.bazaarvoice.emodb.auth.identity.DeferringAuthIdentityManager; +import com.bazaarvoice.emodb.auth.identity.IdentityState; import com.bazaarvoice.emodb.auth.identity.TableAuthIdentityManager; import com.bazaarvoice.emodb.auth.permissions.CacheManagingPermissionManager; import com.bazaarvoice.emodb.auth.permissions.DeferringPermissionManager; @@ -187,11 +188,11 @@ AuthIdentityManager provideAuthIdentityManager( ImmutableList.Builder reservedIdentities = ImmutableList.builder(); reservedIdentities.add( - new ApiKey(replicationKey, REPLICATION_INTERNAL_ID, ImmutableSet.of(DefaultRoles.replication.toString())), - new ApiKey(adminKey, ADMIN_INTERNAL_ID, ImmutableSet.of(DefaultRoles.admin.toString()))); + new ApiKey(replicationKey, REPLICATION_INTERNAL_ID, IdentityState.ACTIVE, ImmutableSet.of(DefaultRoles.replication.toString())), + new ApiKey(adminKey, ADMIN_INTERNAL_ID, IdentityState.ACTIVE, ImmutableSet.of(DefaultRoles.admin.toString()))); if (anonymousKey.isPresent()) { - reservedIdentities.add(new ApiKey(anonymousKey.get(), ANONYMOUS_INTERNAL_ID, ImmutableSet.of(DefaultRoles.anonymous.toString()))); + reservedIdentities.add(new ApiKey(anonymousKey.get(), ANONYMOUS_INTERNAL_ID, IdentityState.ACTIVE, ImmutableSet.of(DefaultRoles.anonymous.toString()))); } AuthIdentityManager deferring = new DeferringAuthIdentityManager<>(daoManager, reservedIdentities.build());