Add support for role namespaces #94

billkalter · 2017-02-21T22:22:29Z

Github Issue

What Are We Doing Here?

As described in the corresponding issue, Emo is moving towards a system of delegated API key management. To fully get there is a four step process:

Create the ability to restrict an API key from creating a role with permissions wider than the API key's current permissions (Change role creation so caller cannot create roles with more permission than himself #63)
Add the ability to group roles together by namespace (Add ability to group roles by namespace #93)
Refactor role and API key management permissions to restrict role management and assignment by group
Expose role and API key management as a public REST interface (Refactor API key and role administration as a public endpoint #50)

This PR addresses the second step in this process. The major changes in this PR are:

Created first-order Role object and the associated management interface and implementations. Each role belongs to exactly one group, including the implicit null group.
Split AuthIdentityManager and PermissionManager interfaces to inherit from more restrictive read-only AuthIdentityReader and PermissionReader interfaces, respectively. This allows authenticators such as ApiKeyRealm to use the reader interfaces without risk of exposing the ability to update users, roles, or permissions. This is not strictly necessary for this PR but with the added interfaces for manipulating security artifacts introduced in this PR it was a good time to close this potential gap.
Refactored PermissionManager to be more agnostic about the permissions it manages. This was done to provide a clean separation of role and permission management since roles are now first-order objects.

Although this PR updates many files it's changes 2 and 3 above which account for a good share of those, due to the class and method name changes percolating through the system.

Design Decisions

It's possible that roles could have been modeled as belonging to multiple groups, not one exclusively. However, this was not done for the following reasons:

There is no use case for it.
It complicates the goal of grouping roles for the purpose of managing permissions to use those roles. If a role belongs to multiple groups the permissions associated with that role become much less clear. For example, if a role belongs to "group1" and "group2" and a user has update permission for roles in "group1" but not "group2" then should he be able to update the role?
If a role can belong to more than one group then the role needs a unique identifier which is separate from the role ID or group, such as a random UUID. While this itself is not a bad thing it requires a complicated dereferencing system and upgrade path with no recognizable benefit.

The unique identifier for a role is derived from the group and a user-provided ID. This means that if a role is deleted and then re-created with the same group and ID then any API keys who had been assigned the role prior to its deletion will still be assigned the role after it is re-created, assuming the role was not explicitly removed from the API key at any time during this process. This is an intentional decision for the following reasons:

It avoids potential confusion given that the (group, ID) is effectively a natural key since the role's creator provides both.
It allows for the assignment of roles to be completely separated from the management of roles. For example, deleting a role does not require scanning every API key and removing the role from that key if present.
It simplifies role management by requiring neither maintaining multiple states for a role nor creating non-natural unique identifiers for new roles upon creation. Both of these solutions add significant complexity for a small benefit which is arguably not the desired behavior.

Note that in addition to (group, ID) the role creator can also set a "name" and "description" attribute. Both of these are informational only and there is no uniqueness guarantee on either of these fields, although it likely benefits the creator to assign unique names to roles within her group.

Upgrade path

The naming convention for roles is as follows:

If the role has no group (group == null) then the role's persistence ID is just the role ID.
If the role has a group then the role's persistence ID is "group/ID".

For this reason, prior to upgrading there must be no roles whose name includes the "/" character. Any such roles should be migrated to new roles with no "/" prior to upgrading.

Once this is done all existing roles are implicitly grandfathered into the null group of roles. All existing API keys and permissions are already persisted in a forward-compatible format. Upgraded Emo servers can be deployed along with the existing ones with no down time or interruption of normal service. The only restriction is that roles should not be modified while the upgrade is taking place.

While the current users, roles, and permissions are forward-compatible the ability to manage existing roles is not. The RebuildMissingRolesTask must be run once per environment after the upgrade to populate the new role persistence tables. Until this is done the administrator will not be able to modify any existing roles.

How to Test and Verify

Check out this PR
Use the RoleAdminTask and ApiKeyAdminTask to create and assign multiple roles, both with and without a group. Verify all works as expected.

Risk

The greatest risk is an error which causes roles to become unstable, since this could lead to users being unable to access parts of Emo for which they should have permission.

Level

Medium While this seems like a large scale PR most of the changes are centered around the management of roles. Permission management is intentionally unaffected to minimize the risk of any changes impacting the existing permissions system.

Required Testing

Manual

Code Review Checklist

Tests are included. If not, make sure you leave us a line or two for the reason.
Pulled down the PR and performed verification of at least being able to
build and run.
Well documented, including updates to any necessary markdown files. When
we inevitably come back to this code it will only take hours to figure out, not
days.
Consistent/Clear/Thoughtful? We are better with this code. We also aren't
a victim of rampaging consistency, and should be using this course of action.
We don't have coding standards out yet for this project, so please make sure to address any feedback regarding STYLE so the codebase remains consistent.
PR has a valid summary, and a good description.

bv-jenkins · 2017-02-21T23:36:46Z

This PR was tested by custom branch build on jenkins

bv-jenkins · 2017-02-22T19:33:55Z

This PR was tested by custom branch build on jenkins

sujithvaddi

@billkalter Looked at the change set. I left few comments.

Just looking at this PR, overall the approach of separating the roles and permissions and their management looks good to me.

sujithvaddi · 2017-03-06T21:41:44Z

web/src/main/java/com/bazaarvoice/emodb/web/auth/SecurityModule.java

+        }
+        return new DeferringRoleManager(delegate, defaultRoles);
+    }
+


I like this intermediate delegate with default roles for performance reasons. So, DataCenterSynchronized manager -> DeferringManagerWithDefaultRoles -> DAOManager 👍

sujithvaddi · 2017-03-06T21:54:59Z

web/src/main/java/com/bazaarvoice/emodb/web/auth/AuthorizationConfiguration.java

@@ -10,6 +10,8 @@
    private final static String DEFAULT_IDENTITY_TABLE = "__auth:keys";
    private final static String DEFAULT_INTERNAL_ID_INDEX_TABLE = "__auth:internal_ids";
    private final static String DEFAULT_PERMISSION_TABLE = "__auth:permissions";
+    private final static String DEFAULT_ROLE_TABLE = "__auth:roles";
+    private final static String DEFAULT_ROLE_GROUP_TABLE = "__auth:role_groups";



Minor: just my thought - you can ignore this...
I see that we are giving the flexibility of having new table names come from config, if not default. But if we want to do that, we will have something else too to deal with as we may be in an error state. So, I was wondering if it hurts to give this flexibility. (If the table names change, they will just be created as new tables in emo and it may not be visible ...)

My thinking is not that we would ever change them. Since Emo is open source different installs may have different naming conventions. I agree, changing the table names on a live system is not supported out-of-the-box and would require outside effort on the administrator's part.

Yeah, I agree - it may be helpful in other projects outside of BV.

sujithvaddi · 2017-03-06T22:00:43Z

.../auth-store/src/main/java/com/bazaarvoice/emodb/auth/permissions/TablePermissionManager.java

-
-    private void update(String key, PermissionUpdateRequest request) {
+    public void updatePermissions(String id, PermissionUpdateRequest request) {
+        checkNotNull(id, "id");
        checkNotNull(request, "request");
        validateTable();



Checking the request's isRevokeRest() is missed here. - is this intentional? I see that the revokeRest property was explicitly added in this PR.

isRevokeRest() is a primitive boolean so it can't be null and either true or false is always valid, so unless there's something deeper I'm missing there's nothing that needs pre-validation here.

My original comment was not about validating. It was about checking the removeRest property and act on it here (to clear the existing data before updating new ones), but now when I look the code again, I see that we are considering that flag when we create and build the Delta. It looks good to me now 👍

sujithvaddi · 2017-03-06T22:03:27Z

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/role/InMemoryRoleManager.java

+        }
+        if (request.isDescriptionPresent()) {
+            role.setDescription(request.getDescription());
+        }


Minor: I know this is a test manager, but just for completeness, can we add this too?
if (request.isNamePresent()) {
role.setName(request.getName());
}

sujithvaddi · 2017-03-06T22:15:33Z

common/api/src/main/java/com/bazaarvoice/emodb/common/api/Names.java

+        return role != null &&
+                role.length() > 0 && role.length() <= 255 &&
+                !role.startsWith("_") &&
+                ROLE_NAME_ALLOWED.matchesAllOf(role);


Minor: why are we just restricting role names not to have _ as first character? why not just not have _ at all OR no such restriction on _ character?
I see no difference between _/_role1 and _/role_1
Also, I see that we using the same method for validating group name (Line 69 in TableRoleManager)(should we generalize the method name or split them if we have separate requirements :?). you think we shouldn't allow _group1/role1 as we may have _/role1 :?

In general Emo uses leading underbars to represent reserved items. I don't mind removing the restriction for role names, but there needs to be protection against "_" as a group name since that is reserved to indicate the absence of a group.

sujithvaddi · 2017-03-06T22:17:56Z

web/src/main/java/com/bazaarvoice/emodb/web/auth/RebuildMissingRolesTask.java

+            }
+        }
+    }
+}


For upgrade purpose and to be used when necessary..nice 👍

sujithvaddi · 2017-03-06T22:22:27Z

auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/role/TableRoleManager.java

+ * RoleManager implementation which persists roles using tables in a {@link DataStore}.
+ */
+public class TableRoleManager implements RoleManager {
+


To make it more obvious that it's the dao layer using datastore api, can we rename this class to TableRoleManagerDAO or TableDAORoleManager ? (Same with TablePermissionManager)

billkalter · 2017-03-07T16:58:27Z

@sujithvaddi Addressed feedback

sujithvaddi · 2017-03-07T17:12:13Z

auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/role/TableRoleManagerDAO.java

-        checkArgument(group == null || isLegalRoleName(group), "Group cannot be named %s", group);
-        // Since legal role names cannot begin with a "_" if the previous check passed then there cannot be a conflict
+        checkArgument(group == null || isLegalRoleGroupName(group), "Group cannot be named %s", group);
+        // Since legal role names cannot equal "_" if the previous check passed then there cannot be a conflict


Minor: legal role group names*

sujithvaddi · 2017-03-07T17:23:14Z

web/src/main/java/com/bazaarvoice/emodb/web/auth/AuthorizationConfiguration.java

@@ -10,6 +10,8 @@
    private final static String DEFAULT_IDENTITY_TABLE = "__auth:keys";
    private final static String DEFAULT_INTERNAL_ID_INDEX_TABLE = "__auth:internal_ids";
    private final static String DEFAULT_PERMISSION_TABLE = "__auth:permissions";
+    private final static String DEFAULT_ROLE_TABLE = "__auth:roles";
+    private final static String DEFAULT_ROLE_GROUP_TABLE = "__auth:role_groups";



Yeah, I agree - it may be helpful in other projects outside of BV.

sujithvaddi

@billkalter the changes are good 👍
However, one test failed in Jenkins - FYI :
system.emodb.tests.core.APIKeyTest.testUpdateRole(APIKeyTest.java:247)

sujithvaddi · 2017-03-07T17:26:20Z

auth/auth-store/src/main/java/com/bazaarvoice/emodb/auth/role/TableRoleManagerDAO.java

-        checkArgument(group == null || isLegalRoleName(group), "Group cannot be named %s", group);
-        // Since legal role names cannot begin with a "_" if the previous check passed then there cannot be a conflict
+        checkArgument(group == null || isLegalRoleGroupName(group), "Group cannot be named %s", group);
+        // Since legal role names cannot equal "_" if the previous check passed then there cannot be a conflict


Minor: Since legal role group names*

bv-jenkins · 2017-03-07T21:03:24Z

This PR was tested by custom branch build on jenkins

bv-jenkins · 2017-03-07T21:41:22Z

This PR was tested by custom branch build on jenkins

billkalter added 2 commits February 10, 2017 14:49

Fixed writes to be global

70025cd

Refactored role management as first order objects organized into groups

4f54a60

billkalter added 3 commits February 22, 2017 10:39

Separated role id into two fields, id and name

6757053

Cleaned up response when updating a role times out

76624d1

Minor cleanup and comments

348510b

sujithvaddi reviewed Mar 6, 2017

View reviewed changes

Addressed feedback

1805131

sujithvaddi reviewed Mar 7, 2017

View reviewed changes

Fixed error in comment

ae511c5

billkalter merged commit 697dc64 into bazaarvoice:master Mar 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for role namespaces #94

Add support for role namespaces #94

billkalter commented Feb 21, 2017 •

edited

Loading

bv-jenkins commented Feb 21, 2017

bv-jenkins commented Feb 22, 2017

sujithvaddi left a comment

sujithvaddi Mar 6, 2017

sujithvaddi Mar 6, 2017

billkalter Mar 7, 2017

sujithvaddi Mar 7, 2017

sujithvaddi Mar 6, 2017

billkalter Mar 7, 2017

sujithvaddi Mar 7, 2017

sujithvaddi Mar 6, 2017

sujithvaddi Mar 6, 2017

billkalter Mar 7, 2017

sujithvaddi Mar 6, 2017

sujithvaddi Mar 6, 2017

billkalter commented Mar 7, 2017

sujithvaddi Mar 7, 2017

sujithvaddi Mar 7, 2017

sujithvaddi left a comment

sujithvaddi Mar 7, 2017

bv-jenkins commented Mar 7, 2017

bv-jenkins commented Mar 7, 2017

Add support for role namespaces #94

Add support for role namespaces #94

Conversation

billkalter commented Feb 21, 2017 • edited Loading

Github Issue

What Are We Doing Here?

How to Test and Verify

Risk

Level

Required Testing

Code Review Checklist

bv-jenkins commented Feb 21, 2017

bv-jenkins commented Feb 22, 2017

sujithvaddi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billkalter commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sujithvaddi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bv-jenkins commented Mar 7, 2017

bv-jenkins commented Mar 7, 2017

billkalter commented Feb 21, 2017 •

edited

Loading