Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactored permissions for roles and API keys #97

Merged
merged 2 commits into from
Mar 14, 2017
Merged

Refactored permissions for roles and API keys #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02e9cbb
Refactored permissions for roles and API keys to provide finder granu…
Feb 28, 2017
14d5ac4
Addressed feedback
Mar 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...h-core/src/main/java/com/bazaarvoice/emodb/auth/identity/InMemoryAuthIdentityManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package com.bazaarvoice.emodb.auth.identity;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Maps;

import java.util.List;
import java.util.Map;
import java.util.Set;

Expand Down Expand Up @@ -47,4 +49,8 @@ public Set<String> getRolesByInternalId(String internalId) {
public void reset() {
_identityMap.clear();
}

public List<T> getAllIdentities() {
return ImmutableList.copyOf(_identityMap.values());
}
}
143 changes: 133 additions & 10 deletions quality/integration/src/test/java/test/integration/auth/ApiKeyAdminTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
import com.bazaarvoice.emodb.common.dropwizard.task.TaskRegistry;
import com.bazaarvoice.emodb.sor.api.DataStore;
import com.bazaarvoice.emodb.web.auth.ApiKeyAdminTask;
import com.bazaarvoice.emodb.web.auth.DefaultRoles;
import com.bazaarvoice.emodb.web.auth.EmoPermissionResolver;
import com.bazaarvoice.emodb.web.auth.Permissions;
import com.bazaarvoice.emodb.web.auth.resource.NamedResource;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.net.HostAndPort;
Expand All @@ -28,6 +28,7 @@

import java.io.PrintWriter;
import java.io.StringWriter;
import java.util.stream.Collectors;

import static org.mockito.Mockito.mock;
import static org.testng.Assert.assertEquals;
Expand All @@ -38,24 +39,34 @@ public class ApiKeyAdminTaskTest {

private ApiKeyAdminTask _task;
private InMemoryAuthIdentityManager<ApiKey> _authIdentityManager;
private InMemoryRoleManager _roleManager;

@BeforeMethod
public void setUp() {
_authIdentityManager = new InMemoryAuthIdentityManager<>();
EmoPermissionResolver permissionResolver = new EmoPermissionResolver(mock(DataStore.class), mock(BlobStore.class));
InMemoryPermissionManager permissionManager = new InMemoryPermissionManager(permissionResolver);
InMemoryRoleManager roleManager = new InMemoryRoleManager(permissionManager);

roleManager.createRole(new RoleIdentifier(null, DefaultRoles.admin.toString()),
new RoleUpdateRequest().withPermissionUpdate(new PermissionUpdateRequest().permit(ImmutableSet.of(Permissions.manageApiKeys()))));
_roleManager = new InMemoryRoleManager(permissionManager);

ApiKeySecurityManager securityManager = new ApiKeySecurityManager(
new ApiKeyRealm("test", new MemoryConstrainedCacheManager(), _authIdentityManager, permissionManager,
null));

_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())));

createApiKeyWithPermissions("test-admin", Permissions.unlimitedApiKey(), Permissions.unlimitedRole());
}

private void createApiKeyWithPermissions(String key, String... permissions) {
String role = key + "-role";
String internalId = key + "-id";

_roleManager.createRole(new RoleIdentifier(null, role),
new RoleUpdateRequest().withPermissionUpdate(new PermissionUpdateRequest().permit(
ImmutableSet.copyOf(permissions))));

_authIdentityManager.updateIdentity(new ApiKey(key, internalId, ImmutableSet.of(role)));
}

@AfterMethod
Expand Down Expand Up @@ -84,6 +95,49 @@ public void testCreateNewApiKey() throws Exception {
assertEquals(apiKey.getDescription(), "desc");
}

@Test
public void testCreateNewApiKeyNoCreatePermission() throws Exception {
createApiKeyWithPermissions("no-create-perm");

StringWriter output = new StringWriter();
PrintWriter pw = new PrintWriter(output);
_task.execute(ImmutableMultimap.<String, String>builder()
.put(ApiKeyRequest.AUTHENTICATION_PARAM, "no-create-perm")
.put("action", "create")
.put("owner", "joe")
.put("description", "desc")
.build(), pw);

assertEquals(output.toString(), "Not authorized\n");
// No new identities except for the two generated by the unit test itself should exist.
assertEquals(
_authIdentityManager.getAllIdentities().stream().map(ApiKey::getId).collect(Collectors.toSet()),
ImmutableSet.of("test-admin", "no-create-perm"));
}

@Test
public void testCreateNewApiKeyInsufficientGrantPermission() throws Exception {
createApiKeyWithPermissions("create-no-grant-perm",
Permissions.createApiKey(),
Permissions.grantRole(new NamedResource("group1")));

StringWriter output = new StringWriter();
PrintWriter pw = new PrintWriter(output);
_task.execute(ImmutableMultimap.<String, String>builder()
.put(ApiKeyRequest.AUTHENTICATION_PARAM, "create-no-grant-perm")
.put("action", "create")
.putAll("role", "group1/allow", "deny")
.put("owner", "joe")
.put("description", "desc")
.build(), pw);

assertEquals(output.toString(), "Not authorized\n");
// No new identities except for the two generated by the unit test itself should exist.
assertEquals(
_authIdentityManager.getAllIdentities().stream().map(ApiKey::getId).collect(Collectors.toSet()),
ImmutableSet.of("test-admin", "create-no-grant-perm"));
}

@Test
public void testUpdateApiKey() throws Exception {
String key = "updateapikeytestkey";
Expand All @@ -103,6 +157,50 @@ public void testUpdateApiKey() throws Exception {
assertEquals(apiKey.getRoles(), ImmutableSet.of("role1", "role2", "role4", "role5"));
}

@Test
public void testUpdateNewApiKeyInsufficientGrantPermissionOnAddedRole() throws Exception {
createApiKeyWithPermissions("update-no-add-grant-perm",
Permissions.grantRole(new NamedResource("group1")));

String key = "updateapikeytestkey";
_authIdentityManager.updateIdentity(new ApiKey(key, "id_update", ImmutableSet.of("role1", "role2", "role3")));

StringWriter output = new StringWriter();
PrintWriter pw = new PrintWriter(output);
_task.execute(ImmutableMultimap.<String, String>builder()
.put(ApiKeyRequest.AUTHENTICATION_PARAM, "update-no-add-grant-perm")
.put("action", "update")
.put("key", key)
.putAll("addRole", "group2/role4")
.build(), pw);

assertEquals(output.toString(), "Not authorized\n");
// Roles should be unchanged
assertEquals(_authIdentityManager.getIdentity(key).getRoles(), ImmutableSet.of("role1", "role2", "role3"));
}

@Test
public void testUpdateNewApiKeyInsufficientGrantPermissionOnRemovedRole() throws Exception {
createApiKeyWithPermissions("update-no-remove-grant-perm",
Permissions.grantRole(new NamedResource("group1")));

String key = "updateapikeytestkey";
_authIdentityManager.updateIdentity(new ApiKey(key, "id_update", ImmutableSet.of("role1", "role2", "role3")));

StringWriter output = new StringWriter();
PrintWriter pw = new PrintWriter(output);
_task.execute(ImmutableMultimap.<String, String>builder()
.put(ApiKeyRequest.AUTHENTICATION_PARAM, "update-no-remove-grant-perm")
.put("action", "update")
.put("key", key)
.putAll("removeRole", "role1")
.build(), pw);

assertEquals(output.toString(), "Not authorized\n");
// Roles should be unchanged
assertEquals(_authIdentityManager.getIdentity(key).getRoles(), ImmutableSet.of("role1", "role2", "role3"));
}

@Test
public void testMigrateApiKey() throws Exception {
String key = "migrateapikeytestkey";
Expand All @@ -124,6 +222,25 @@ public void testMigrateApiKey() throws Exception {
assertNull(_authIdentityManager.getIdentity(key));
}

@Test
public void testMigrateNewApiKeyNoUpdatePermission() throws Exception {
createApiKeyWithPermissions("no-update-perm");

String key = "migrateapikeytestkey";
_authIdentityManager.updateIdentity(new ApiKey(key, "id_migrate", ImmutableSet.of("role1", "role2")));
assertNotNull(_authIdentityManager.getIdentity(key));

StringWriter output = new StringWriter();
PrintWriter pw = new PrintWriter(output);
_task.execute(ImmutableMultimap.of(
ApiKeyRequest.AUTHENTICATION_PARAM, "no-update-perm",
"action", "migrate", "key", key), pw);

assertEquals(output.toString(), "Not authorized\n");
// API key should be unchanged
assertNotNull(_authIdentityManager.getIdentity(key));
}

@Test
public void testDeleteApiKey() throws Exception {
String key = "deleteapikeytestkey";
Expand All @@ -138,15 +255,21 @@ public void testDeleteApiKey() throws Exception {
}

@Test
public void testBadAdminCredentials() throws Exception {
public void testDeleteApiKeyNoDeletePermission() throws Exception {
createApiKeyWithPermissions("no-delete-perm");

String key = "deleteapikeytestkey";
_authIdentityManager.updateIdentity(new ApiKey(key, "id_delete", ImmutableSet.of("role1", "role2")));
assertNotNull(_authIdentityManager.getIdentity(key));

StringWriter output = new StringWriter();
PrintWriter pw = new PrintWriter(output);

_task.execute(ImmutableMultimap.of(
ApiKeyRequest.AUTHENTICATION_PARAM, "invalid-api-key",
"action", "delete", "key", "somekey"), pw);
ApiKeyRequest.AUTHENTICATION_PARAM, "no-delete-perm",
"action", "delete", "key", key), pw);

assertEquals(output.toString(), "Not authorized\n");
assertNotNull(_authIdentityManager.getIdentity(key));
}

@Test (expectedExceptions = IllegalArgumentException.class)
Expand Down
2 changes: 1 addition & 1 deletion quality/integration/src/test/java/test/integration/auth/RoleAdminTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public void setUp() {

RoleIdentifier adminId = new RoleIdentifier(null, DefaultRoles.admin.toString());
_roleManager.createRole(adminId, new RoleUpdateRequest()
.withPermissionUpdate(new PermissionUpdateRequest().permit(ImmutableSet.of(Permissions.manageRoles()))));
.withPermissionUpdate(new PermissionUpdateRequest().permit(ImmutableSet.of(Permissions.unlimitedRole()))));
ApiKeySecurityManager securityManager = new ApiKeySecurityManager(
new ApiKeyRealm("test", new MemoryConstrainedCacheManager(), _authIdentityManager, permissionManager,
null));
Expand Down
65 changes: 48 additions & 17 deletions web/src/main/java/com/bazaarvoice/emodb/web/auth/ApiKeyAdminTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +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.role.RoleIdentifier;
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.bazaarvoice.emodb.web.auth.resource.VerifiableResource;
import com.google.common.base.Joiner;
import com.google.common.base.Optional;
import com.google.common.base.Predicates;
Expand All @@ -20,8 +22,8 @@
import com.google.common.primitives.Longs;
import com.google.inject.Inject;
import io.dropwizard.servlets.tasks.Task;
import org.apache.cassandra.thrift.AuthorizationException;
import org.apache.shiro.authc.AuthenticationException;
import org.apache.shiro.authz.AuthorizationException;
import org.apache.shiro.mgt.SecurityManager;
import org.apache.shiro.subject.Subject;
import org.slf4j.Logger;
Expand All @@ -41,8 +43,10 @@
* Task for managing API keys and the roles associated with them.
*
* The following examples demonstrate the various ways to use this task. In order to actually run this task you must
* provide an API key which has permission to manage API keys (see {@link Permissions#manageApiKeys()}). For
* the purposes of this example the API key "admin-key" is a valid key with this permission.
* provide an API key which has permission to perform the action requested, such as {@link Permissions#updateApiKey()}.
* Additionally, in order to grant or revoke roles for an API key you must provide an API key with grant permission
* for those roles, such as {@link Permissions#grantRole(VerifiableResource, VerifiableResource)}.
* For the purposes of this example the API key "admin-key" is a valid key with all required permissions.
*
* Create API key
* ==============
Expand Down Expand Up @@ -138,28 +142,25 @@ public void execute(ImmutableMultimap<String, String> parameters, PrintWriter ou
// Make sure the API key is valid
String apiKey = getValueFromParams(ApiKeyRequest.AUTHENTICATION_PARAM, parameters);
subject.login(new ApiKeyAuthenticationToken(apiKey));

// Make sure the API key is permitted to manage API keys
subject.checkPermission(Permissions.manageApiKeys());


String activityStr = getValueFromParams("action", parameters);
Action action = Action.valueOf(activityStr.toUpperCase());

switch (action) {
case CREATE:
createApiKey(parameters, output);
createApiKey(subject, parameters, output);
break;
case VIEW:
viewApiKey(parameters, output);
viewApiKey(subject, parameters, output);
break;
case UPDATE:
updateApiKey(parameters, output);
updateApiKey(subject, parameters, output);
break;
case MIGRATE:
migrateApiKey(parameters, output);
migrateApiKey(subject, parameters, output);
break;
case DELETE:
deleteApiKey(parameters, output);
deleteApiKey(subject, parameters, output);
break;
}
} catch (AuthenticationException | AuthorizationException e) {
Expand All @@ -179,12 +180,19 @@ private String createUniqueInternalId() {
return BaseEncoding.base32().omitPadding().encode(b);
}

private void createApiKey(ImmutableMultimap<String, String> parameters, PrintWriter output)
private void createApiKey(Subject subject, ImmutableMultimap<String, String> parameters, PrintWriter output)
throws Exception {
String owner = getValueFromParams("owner", parameters);
Set<String> roles = ImmutableSet.copyOf(parameters.get("role"));
String description = Iterables.getFirst(parameters.get("description"), null);

// Does the caller have permission to create API keys?
subject.checkPermission(Permissions.createApiKey());
// Does the caller have permission to grant each role provided?
for (String role : roles) {
subject.checkPermission(Permissions.grantRole(RoleIdentifier.fromString(role)));
}

// 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.
Expand Down Expand Up @@ -274,8 +282,9 @@ private boolean isProvidedApiKeyValid(String apiKey) {
return Pattern.matches("[a-zA-Z0-9]{48}", apiKey);
}

private void viewApiKey(ImmutableMultimap<String, String> parameters, PrintWriter output)
private void viewApiKey(Subject subject, ImmutableMultimap<String, String> parameters, PrintWriter output)
throws Exception {
subject.checkPermission(Permissions.readApiKey());
String key = getValueFromParams("key", parameters);

ApiKey apiKey = _authIdentityManager.getIdentity(key);
Expand All @@ -289,13 +298,21 @@ private void viewApiKey(ImmutableMultimap<String, String> parameters, PrintWrite
}
}

private void updateApiKey(ImmutableMultimap<String, String> parameters, PrintWriter output) {
private void updateApiKey(Subject subject, ImmutableMultimap<String, String> parameters, PrintWriter output) {
Set<String> addRoles = ImmutableSet.copyOf(parameters.get("addRole"));
Set<String> removeRoles = ImmutableSet.copyOf(parameters.get("removeRole"));

checkArgument(!addRoles.isEmpty() || !removeRoles.isEmpty(), "Update requires one or more 'addRole' or 'removeRole' parameters");
checkArgument(Sets.intersection(addRoles, _reservedRoles).isEmpty(), "Cannot assign reserved role");

// Does the caller have permission to grant all roles being added or removed?
for (String role : addRoles) {
subject.checkPermission(Permissions.grantRole(RoleIdentifier.fromString(role)));
}
for (String role : removeRoles) {
subject.checkPermission(Permissions.grantRole(RoleIdentifier.fromString(role)));
}

String key = getValueFromParams("key", parameters);
ApiKey apiKey = _authIdentityManager.getIdentity(key);
checkArgument(apiKey != null, "Unknown API key");
Expand All @@ -317,7 +334,10 @@ private void updateApiKey(ImmutableMultimap<String, String> parameters, PrintWri
output.println("API key updated");
}

private void migrateApiKey(ImmutableMultimap<String, String> parameters, PrintWriter output) {
private void migrateApiKey(Subject subject, ImmutableMultimap<String, String> parameters, PrintWriter output) {
// Migrating a key is considered a key update operation, so check for update permission
subject.checkPermission(Permissions.updateApiKey());

String key = getValueFromParams("key", parameters);
ApiKey apiKey = _authIdentityManager.getIdentity(key);
checkArgument(apiKey != null, "Unknown API key");
Expand All @@ -332,8 +352,19 @@ private void migrateApiKey(ImmutableMultimap<String, String> parameters, PrintWr
output.println("\nWarning: This is your only chance to see this key. Save it somewhere now.");
}

private void deleteApiKey(ImmutableMultimap<String, String> parameters, PrintWriter output) {
private void deleteApiKey(Subject subject, ImmutableMultimap<String, String> parameters, PrintWriter output) {
// Does the caller have permission to delete API keys?
subject.checkPermission(Permissions.deleteApiKey());

String key = getValueFromParams("key", parameters);
ApiKey apiKey = _authIdentityManager.getIdentity(key);
checkArgument(apiKey != null, "Unknown API key");

// Does the caller have permission to revoke every role from the API key?
for (String role : apiKey.getRoles()) {
subject.checkPermission(Permissions.grantRole(RoleIdentifier.fromString(role)));
}

_authIdentityManager.deleteIdentity(key);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@billkalter Looks good. 👍

output.println("API key deleted");
}
Expand Down
Loading