From 02e9cbb2d5f97430e62593efa6e51ba108396dbe Mon Sep 17 00:00:00 2001 From: billkalter Date: Tue, 28 Feb 2017 13:09:53 -0600 Subject: [PATCH 1/2] Refactored permissions for roles and API keys to provide finder granularity based on role group. --- .../identity/InMemoryAuthIdentityManager.java | 6 + .../integration/auth/ApiKeyAdminTaskTest.java | 143 ++++++++++++++++-- .../integration/auth/RoleAdminTaskTest.java | 2 +- .../emodb/web/auth/ApiKeyAdminTask.java | 55 ++++--- .../emodb/web/auth/EmoPermission.java | 17 ++- .../emodb/web/auth/Permissions.java | 116 ++++++++++++-- .../emodb/web/auth/RoleAdminTask.java | 47 +++--- 7 files changed, 326 insertions(+), 60 deletions(-) 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..ac960f8f14 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,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; @@ -47,4 +49,8 @@ public Set getRolesByInternalId(String internalId) { public void reset() { _identityMap.clear(); } + + public List getAllIdentities() { + return ImmutableList.copyOf(_identityMap.values()); + } } 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 8da83ca01a..b3e3846f22 100644 --- a/quality/integration/src/test/java/test/integration/auth/ApiKeyAdminTaskTest.java +++ b/quality/integration/src/test/java/test/integration/auth/ApiKeyAdminTaskTest.java @@ -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; @@ -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; @@ -38,16 +39,14 @@ public class ApiKeyAdminTaskTest { private ApiKeyAdminTask _task; private InMemoryAuthIdentityManager _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, @@ -55,7 +54,19 @@ 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()))); + + 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 @@ -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.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.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"; @@ -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.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.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"; @@ -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"; @@ -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) 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 86c21fd4a4..4e7d8f8632 100644 --- a/quality/integration/src/test/java/test/integration/auth/RoleAdminTaskTest.java +++ b/quality/integration/src/test/java/test/integration/auth/RoleAdminTaskTest.java @@ -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)); 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..1e0bb80f5e 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,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; @@ -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; @@ -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 * ============== @@ -138,28 +142,25 @@ public void execute(ImmutableMultimap 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) { @@ -179,12 +180,19 @@ private String createUniqueInternalId() { return BaseEncoding.base32().omitPadding().encode(b); } - private void createApiKey(ImmutableMultimap parameters, PrintWriter output) + private void createApiKey(Subject subject, 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); + // 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. @@ -274,8 +282,9 @@ private boolean isProvidedApiKeyValid(String apiKey) { return Pattern.matches("[a-zA-Z0-9]{48}", apiKey); } - private void viewApiKey(ImmutableMultimap parameters, PrintWriter output) + private void viewApiKey(Subject subject, ImmutableMultimap parameters, PrintWriter output) throws Exception { + subject.checkPermission(Permissions.readApiKey()); String key = getValueFromParams("key", parameters); ApiKey apiKey = _authIdentityManager.getIdentity(key); @@ -289,13 +298,21 @@ private void viewApiKey(ImmutableMultimap parameters, PrintWrite } } - private void updateApiKey(ImmutableMultimap parameters, PrintWriter output) { + private void updateApiKey(Subject subject, ImmutableMultimap parameters, PrintWriter output) { Set addRoles = ImmutableSet.copyOf(parameters.get("addRole")); Set 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"); @@ -317,7 +334,10 @@ private void updateApiKey(ImmutableMultimap parameters, PrintWri output.println("API key updated"); } - private void migrateApiKey(ImmutableMultimap parameters, PrintWriter output) { + private void migrateApiKey(Subject subject, ImmutableMultimap 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"); @@ -332,7 +352,8 @@ private void migrateApiKey(ImmutableMultimap parameters, PrintWr output.println("\nWarning: This is your only chance to see this key. Save it somewhere now."); } - private void deleteApiKey(ImmutableMultimap parameters, PrintWriter output) { + private void deleteApiKey(Subject subject, ImmutableMultimap parameters, PrintWriter output) { + subject.checkPermission(Permissions.deleteApiKey()); String key = getValueFromParams("key", parameters); _authIdentityManager.deleteIdentity(key); output.println("API key deleted"); diff --git a/web/src/main/java/com/bazaarvoice/emodb/web/auth/EmoPermission.java b/web/src/main/java/com/bazaarvoice/emodb/web/auth/EmoPermission.java index f4dc60d68c..9cfc57f75d 100644 --- a/web/src/main/java/com/bazaarvoice/emodb/web/auth/EmoPermission.java +++ b/web/src/main/java/com/bazaarvoice/emodb/web/auth/EmoPermission.java @@ -20,6 +20,7 @@ import java.util.List; import static com.google.common.base.Preconditions.checkArgument; +import static java.lang.String.format; /** * Permission used by EmoDB for authorization. It is identical to {@link MatchingPermission} except that it allows @@ -34,7 +35,7 @@ * table template. * * - * EmoPermission checkedPermission = new EmoPermission(dataStore, "sor|update|table(review:testcustomer)"); + * EmoPermission checkedPermission = new EmoPermission(dataStore, "sor|update|review:testcustomer"); * Map<String, Object> template = dataStore.getTableTemplate("review:testcustomer"); * * assert userPermission.implies(checkedPermission) == (template.get("team").equals("EmoDB") && template.get("type").equals("review")); @@ -46,11 +47,13 @@ public class EmoPermission extends MatchingPermission { private final static ConstantPart SOR_PART = new EmoConstantPart(Permissions.SOR); // All blob store related permissions have this as the first part private final static ConstantPart BLOB_PART = new EmoConstantPart(Permissions.BLOB); + // All role related permissions have this as the first part + private final static ConstantPart ROLE_PART = new EmoConstantPart(Permissions.ROLE); private final DataStore _dataStore; private final BlobStore _blobStore; - private static enum PartType { + private enum PartType { CONTEXT, ACTION, SOR_TABLE, @@ -108,9 +111,17 @@ protected MatchingPart toPart(List leadingParts, String part) { partType = PartType.NAMED_RESOURCE; } return createEmoPermissionPart(part, partType); + + case 3: + // Only roles support four parts, where the group is the third part and the role ID is the fourth part. + if (MatchingPart.contextImpliedBy(ROLE_PART, leadingParts)) { + return createEmoPermissionPart(part, PartType.NAMED_RESOURCE); + } + break; } - throw new IllegalArgumentException("EmoPermission does not support over three parts"); + throw new IllegalArgumentException(format("Too many parts for EmoPermission in \"%s\" context", + MatchingPart.getContext(leadingParts))); } /** diff --git a/web/src/main/java/com/bazaarvoice/emodb/web/auth/Permissions.java b/web/src/main/java/com/bazaarvoice/emodb/web/auth/Permissions.java index a6ab3e15ea..588892a978 100644 --- a/web/src/main/java/com/bazaarvoice/emodb/web/auth/Permissions.java +++ b/web/src/main/java/com/bazaarvoice/emodb/web/auth/Permissions.java @@ -1,11 +1,16 @@ package com.bazaarvoice.emodb.web.auth; +import com.bazaarvoice.emodb.auth.role.RoleIdentifier; import com.bazaarvoice.emodb.sor.api.Intrinsic; import com.bazaarvoice.emodb.sor.condition.Conditions; import com.bazaarvoice.emodb.web.auth.resource.AnyResource; import com.bazaarvoice.emodb.web.auth.resource.ConditionResource; import com.bazaarvoice.emodb.web.auth.resource.CreateTableResource; +import com.bazaarvoice.emodb.web.auth.resource.NamedResource; import com.bazaarvoice.emodb.web.auth.resource.VerifiableResource; +import com.google.common.base.Objects; + +import javax.annotation.Nullable; import static com.bazaarvoice.emodb.auth.permissions.MatchingPermission.escapeSeparators; import static java.lang.String.format; @@ -18,11 +23,15 @@ public class Permissions { public final static String BLOB = "blob"; public final static String QUEUE = "queue"; public final static String DATABUS = "databus"; + public final static String ROLE = "role"; + public final static String API_KEY = "apikey"; public final static String SYSTEM = "system"; // Actions public final static String READ = "read"; + public final static String CREATE = "create"; public final static String UPDATE = "update"; + public final static String DELETE = "delete"; public final static String CREATE_TABLE = "create_table"; public final static String CREATE_FACADE = "create_facade"; public final static String SET_TABLE_ATTRIBUTES = "set_table_attributes"; @@ -39,8 +48,7 @@ public class Permissions { public final static String PURGE = "purge"; public final static String REPLICATE_DATABUS = "replicate_databus"; public final static String RAW_DATABUS = "raw_databus"; - public final static String MANAGE_API_KEYS = "manage_api_keys"; - public final static String MANAGE_ROLES = "manage_roles"; + public final static String GRANT = "grant"; // Common resource values public final static AnyResource ALL = new AnyResource(); @@ -205,6 +213,101 @@ public static String unlimitedDatabus(VerifiableResource subscription) { return format("%s|%s|%s", DATABUS, ALL, escapeSeparators(subscription.toString())); } + // Authentication and authorization permissions + + /** + * By convention permissions for roles with no group use "_" as the group name for permissions. + */ + public static VerifiableResource toRoleGroupResource(@Nullable String group) { + return new NamedResource(Objects.firstNonNull(group, "_")); + } + + public static String readRole(VerifiableResource group) { + return readRole(group, ALL); + } + + public static String readRole(VerifiableResource group, VerifiableResource role) { + return format("%s|%s|%s|%s", ROLE, READ, escapeSeparators(group.toString()), escapeSeparators(role.toString())); + } + + public static String readRole(RoleIdentifier roleIdentifier) { + return readRole(toRoleGroupResource(roleIdentifier.getGroup()), new NamedResource(roleIdentifier.getId())); + } + + public static String createRole(VerifiableResource group) { + return createRole(group, ALL); + } + + public static String createRole(VerifiableResource group, VerifiableResource role) { + return format("%s|%s|%s|%s", ROLE, CREATE, escapeSeparators(group.toString()), escapeSeparators(role.toString())); + } + + public static String createRole(RoleIdentifier roleIdentifier) { + return createRole(toRoleGroupResource(roleIdentifier.getGroup()), new NamedResource(roleIdentifier.getId())); + } + + public static String updateRole(VerifiableResource group) { + return updateRole(group, ALL); + } + + public static String updateRole(VerifiableResource group, VerifiableResource role) { + return format("%s|%s|%s|%s", ROLE, UPDATE, escapeSeparators(group.toString()), escapeSeparators(role.toString())); + } + + public static String updateRole(RoleIdentifier roleIdentifier) { + return updateRole(toRoleGroupResource(roleIdentifier.getGroup()), new NamedResource(roleIdentifier.getId())); + } + + public static String deleteRole(VerifiableResource group) { + return deleteRole(group, ALL); + } + + public static String deleteRole(RoleIdentifier roleIdentifier) { + return deleteRole(toRoleGroupResource(roleIdentifier.getGroup()), new NamedResource(roleIdentifier.getId())); + } + + public static String deleteRole(VerifiableResource group, VerifiableResource role) { + return format("%s|%s|%s|%s", ROLE, DELETE, escapeSeparators(group.toString()), escapeSeparators(role.toString())); + } + + public static String grantRole(VerifiableResource group) { + return grantRole(group, ALL); + } + + public static String grantRole(VerifiableResource group, VerifiableResource role) { + return format("%s|%s|%s|%s", ROLE, GRANT, escapeSeparators(group.toString()), escapeSeparators(role.toString())); + } + + public static String grantRole(RoleIdentifier roleIdentifier) { + return grantRole(toRoleGroupResource(roleIdentifier.getGroup()), new NamedResource(roleIdentifier.getId())); + } + + public static String unlimitedRole() { + return ROLE; + } + + public static String readApiKey() { + return format("%s|%s", API_KEY, READ); + } + + public static String createApiKey() { + return format("%s|%s", API_KEY, CREATE); + } + + public static String updateApiKey() { + return format("%s|%s", API_KEY, UPDATE); + } + + public static String deleteApiKey() { + return format("%s|%s", API_KEY, DELETE); + } + + public static String unlimitedApiKey() { + return API_KEY; + } + + // System permissions + /** * Although the following permission concerns the databus it is placed in the "system" resource since * databus replication should only be performed internally by the system. @@ -220,13 +323,4 @@ public static String replicateDatabus() { public static String rawDatabus() { return format("%s|%s", SYSTEM, RAW_DATABUS); } - - public static String manageApiKeys() { - return format("%s|%s", SYSTEM, MANAGE_API_KEYS); - } - - public static String manageRoles() { - return format("%s|%s", SYSTEM, MANAGE_ROLES); - } - } diff --git a/web/src/main/java/com/bazaarvoice/emodb/web/auth/RoleAdminTask.java b/web/src/main/java/com/bazaarvoice/emodb/web/auth/RoleAdminTask.java index 3a8efe9eb4..99d9af1300 100644 --- a/web/src/main/java/com/bazaarvoice/emodb/web/auth/RoleAdminTask.java +++ b/web/src/main/java/com/bazaarvoice/emodb/web/auth/RoleAdminTask.java @@ -8,6 +8,7 @@ import com.bazaarvoice.emodb.auth.role.RoleManager; import com.bazaarvoice.emodb.auth.role.RoleUpdateRequest; import com.bazaarvoice.emodb.common.dropwizard.task.TaskRegistry; +import com.bazaarvoice.emodb.web.auth.resource.VerifiableResource; import com.google.common.base.Strings; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; @@ -41,8 +42,9 @@ * Task for managing roles and the permissions 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 roles (see {@link Permissions#manageRoles()}). 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#updateRole(VerifiableResource, VerifiableResource)}. For the purposes of this example the API key + * "admin-key" is a valid key with all required permissions. * * Create or update role * ===================== @@ -133,27 +135,25 @@ public void execute(ImmutableMultimap parameters, PrintWriter ou String apiKey = getValueFromParams(ApiKeyRequest.AUTHENTICATION_PARAM, parameters); subject.login(new ApiKeyAuthenticationToken(apiKey)); - // Make sure the API key is permitted to manage roles - subject.checkPermission(Permissions.manageRoles()); - String activityStr = getValueFromParams("action", parameters); Action action = Action.valueOf(activityStr.toUpperCase().replace('-', '_')); + RoleIdentifier roleId = getRole(parameters); switch (action) { case VIEW: - viewRole(getRole(parameters), output); + viewRole(subject, roleId, output); break; case UPDATE: - createOrUpdateRole(getRole(parameters), parameters, output); + createOrUpdateRole(subject, roleId, parameters, output); break; case DELETE: - deleteRole(getRole(parameters), output); + deleteRole(subject, roleId, output); break; case CHECK: - checkPermission(getRole(parameters), parameters, output); + checkPermission(subject, roleId, parameters, output); break; case FIND_DEPRECATED_PERMISSIONS: - findDeprecatedPermissions(output); + findDeprecatedPermissions(subject, output); } } catch (AuthenticationException | AuthorizationException e) { _log.warn("Unauthorized attempt to access role management task"); @@ -182,7 +182,8 @@ private RoleIdentifier getRole(ImmutableMultimap parameters) { return new RoleIdentifier(group, ids.get(0)); } - private void viewRole(RoleIdentifier id, PrintWriter output) { + private void viewRole(Subject subject, RoleIdentifier id, PrintWriter output) { + subject.checkPermission(Permissions.readRole(id)); Set permissions = _roleManager.getPermissionsForRole(id); output.println(String.format("%s has %d permissions", id, permissions.size())); for (String permission : permissions) { @@ -190,7 +191,8 @@ private void viewRole(RoleIdentifier id, PrintWriter output) { } } - private void createOrUpdateRole(RoleIdentifier id, ImmutableMultimap parameters, PrintWriter output) { + private void createOrUpdateRole(Subject subject, RoleIdentifier id, ImmutableMultimap parameters, + PrintWriter output) { checkArgument(!DefaultRoles.isDefaultRole(id), "Cannot update default role: %s", id); String name = parameters.get("name").stream().findFirst().orElse(null); @@ -239,22 +241,27 @@ private void createOrUpdateRole(RoleIdentifier id, ImmutableMultimap parameters, PrintWriter output) { + private void checkPermission(Subject subject, RoleIdentifier id, ImmutableMultimap parameters, PrintWriter output) { + subject.checkPermission(Permissions.readRole(id)); + String permissionStr = getValueFromParams("permission", parameters); final Permission permission = _permissionResolver.resolvePermission(permissionStr); @@ -274,7 +281,11 @@ private void checkPermission(RoleIdentifier id, ImmutableMultimap { From 14d5ac470dfb19b08cc08eea62d5bbb1a253dcdd Mon Sep 17 00:00:00 2001 From: billkalter Date: Mon, 13 Mar 2017 15:02:41 -0500 Subject: [PATCH 2/2] Addressed feedback --- .../bazaarvoice/emodb/web/auth/ApiKeyAdminTask.java | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 1e0bb80f5e..36295ce62a 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 @@ -353,8 +353,18 @@ private void migrateApiKey(Subject subject, ImmutableMultimap pa } private void deleteApiKey(Subject subject, ImmutableMultimap 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); output.println("API key deleted"); }