Skip to content

Commit

Permalink
Added internal ID tracking and improved cache management for API keys
Browse files Browse the repository at this point in the history
  • Loading branch information
billkalter committed Sep 22, 2016
1 parent cfdbeac commit 99b44c9
Show file tree
Hide file tree
Showing 37 changed files with 1,127 additions and 144 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.bazaarvoice.emodb.auth;

import org.apache.shiro.mgt.SecurityManager;

/**
* Extension of the {@link SecurityManager} interface which adds methods for verifying permissions by internal ID
* for users not currently authenticated.
*/
public interface EmoSecurityManager extends SecurityManager, InternalAuthorizer {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package com.bazaarvoice.emodb.auth;

import org.apache.shiro.authz.Permission;

import javax.annotation.Nullable;

/**
* Interface for performing authorization internally within the system. Unlike SecurityManager this interface is
* intended to be used primarily in contexts where the user is not authenticated. The interface is intentionally
* limited to discourage bypassing the SecurityManager when dealing with authenticated users.
*
* Internal systems are encouraged to identify relationships such as resource ownership with internal IDs instead of
* public credentials like API keys for the following reasons:
*
* <ul>
* <li>If the API key for a user is changed the internal ID remains constant.</li>
* <li>They can safely log and store the internal ID of a user without risk of leaking plaintext credentials.</li>
* </ul>
*/
public interface InternalAuthorizer {

boolean hasPermissionByInternalId(String internalId, String permission);

boolean hasPermissionByInternalId(String internalId, Permission permission);

boolean hasPermissionsByInternalId(String internalId, String... permissions);

boolean hasPermissionsByInternalId(String internalId, Permission... permissions);
}
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public SecurityManagerBuilder withAnonymousAccessAs(@Nullable String id) {
return this;
}

public SecurityManager build() {
public EmoSecurityManager build() {
checkNotNull(_authIdentityManager, "authIdentityManager not set");
checkNotNull(_permissionManager, "permissionManager not set");
if(_cacheManager == null) { // intended for test use
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.collect.ImmutableSet;
import com.google.common.hash.Hashing;

import java.util.List;
import java.util.Set;
Expand All @@ -13,12 +14,29 @@
*/
public class ApiKey extends AuthIdentity {

public ApiKey(String key, Set<String> roles) {
super(key, roles);
public ApiKey(String key, String internalId, Set<String> roles) {
super(key, internalId, roles);
}

@JsonCreator
public ApiKey(@JsonProperty("id") String key, @JsonProperty("roles") List<String> roles) {
this(key, ImmutableSet.copyOf(roles));
public ApiKey(@JsonProperty("id") String key,
@JsonProperty("internalId") String internalId,
@JsonProperty("roles") List<String> roles) {

// API keys have been in use since before internal IDs were introduced. To grandfather in those keys we'll
// use a hash of the API key as the internal ID.
this(key, resolveInternalId(key, internalId), ImmutableSet.copyOf(roles));
}

private static String resolveInternalId(String key, String internalId) {
if (internalId != null) {
return internalId;
}
// SHA-256 is a little heavy but it has two advantages:
// 1. It is the same algorithm currently used to store API keys by the permission manager so there isn't a
// potential conflict between keys.
// 2. The API keys are cached by Shiro so this conversion will only take place when the key needs to
// be (re)loaded.
return Hashing.sha256().hashUnencodedChars(key).toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ public class ApiKeyAuthenticationInfo implements AuthenticationInfo {
public ApiKeyAuthenticationInfo(ApiKey apiKey, String realm) {
checkNotNull(apiKey, "apiKey");
checkNotNull(realm, "realm");
PrincipalWithRoles principal = new PrincipalWithRoles(apiKey.getId(), apiKey.getRoles());
// Identify the principal by API key
PrincipalWithRoles principal = new PrincipalWithRoles(apiKey.getId(), apiKey.getInternalId(), apiKey.getRoles());
_principals = new SimplePrincipalCollection(principal, realm);
// Use the API key as the credentials
_credentials = apiKey.getId();
Expand All @@ -40,4 +41,23 @@ public String getCredentials() {
public String toString() {
return format("%s{%s}", getClass().getSimpleName(), ((PrincipalWithRoles) _principals.getPrimaryPrincipal()).getName());
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof ApiKeyAuthenticationInfo)) {
return false;
}

ApiKeyAuthenticationInfo that = (ApiKeyAuthenticationInfo) o;

return _principals.equals(that._principals) && _credentials.equals(that._credentials);
}

@Override
public int hashCode() {
return _principals.getPrimaryPrincipal().hashCode();
}
}
Loading

0 comments on commit 99b44c9

Please sign in to comment.