Refactored permissions for roles and API keys

[billkalter]

Github Issue

None

What Are We Doing Here?

Currently the ability to create roles, create API keys, and assign roles to API keys are all managed by the following wide permissions:

• system|manage_roles can CRUD any role
• system|manage_api_keys can CRUD any API key and assign or unassign any role from any API key

With the move to delegated authentication these permissions are being replaced with much narrower ones:

• CRUD permissions for roles, such as role|read, which can be restricted by role group and then further restricted by ID. For example, role|*|team_x|* grants full permissions on any role in the "team_x" group and no permissions on any role outside that group.
• Grant permissions for roles. A user with grant permission for a role can assign or unassign that role to any API key. For example, role|grant|team_x|* grants permission to add or remove any role in group "team_x" to or from any API key.
• CRUD permissions for API keys, such as apikey|read. Since API keys are randomly generated and there is currently no concept of grouping API keys by any attribute these provide blanket permissions to act on any API key. For example, apikey|read grants the ability to read all API keys, though other permissions would be necessary create, update, or delete an API key.

Several notable decisions made in this PR:

• The ability to grant or revoke a role from an API key are both controlled by the single permission role|grant|.... There didn't seem to be a use case where a user could grant a role but not be able to revoke it, or vice versa, so separating them would be an unnecessary complication.
• The ability to add or create permissions to a role does not distinguish which permissions to act upon. For example, user A could add permission P to a role and then user B could revoke P from the role, so long as both have update permission on the role. The reasoning is that permissions control who can update which roles and it is up to the limited set of users who have that permission on that role to coordinate with each other. Otherwise role management becomes tedious and untenable.
• The ability to control which permissions can be granted to a role by a user is not part of this PR but is part of PR Permission subset checks when creating roles #64.
• Since nulls cannot be represented in permissions the already reserved role group name "_" is used in permissions for the null group, such as role|read|_|*
• It's possible that further granularity could be added to API key permissions, such as restricting by owner or some new set of API key attributes. However, if that were to be done it would be part of a subsequent PR.

How to Test and Verify

1. Check out this PR
2. As administrator, create numerous API keys with varying role and apikey permissions.
3. Verify each only can perform those administrative tasks for which it has permission.

Risk

Risk is relatively low. Currently only administrators can perform these tasks anyway, so worst case if there is an issue it won't be client-facing. The actual validation of all existing permissions is unchanged so there is no risk of breaking existing non-administrative API keys or their ability to interact normally with EmoDB.

Level

Medium

Required Testing

Manual

Risk Summary

The greatest risk is that the new permissions don't work as expected and lock out administrative access. However, even were that to happen it's not client-facing and can be worked around until a fix version is released.

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.

[sujithvaddi]

@billkalter I left a comment. Otherwise the change looks good to me.

[sujithvaddi]

I know that the user will not provide the roles when deleting the api-key, so no roles in the parameters input, but do you think we need to pull up all the roles of the api-key and do a subject.checkPermission(Permissions.grantRole(RoleIdentifier.fromString(role)));
for each role before deleting the api-key:?

[billkalter]

That's a great question. My reasoning is this:

1. The grant-role check when removing a role from an API key is to prevent an accidental or possibly malicious user from revoking permissions which he himself cannot grant back. Basically, the grant-role permission is being used as a proxy for group ownership.
2. When an API key is being deleted there is no API key left for which the role can be granted back. Therefore, rather than requiring permission to grant each individual role a much higher and more protected delete-api-key permission is needed.

It's possible that an API key can exist which has delete-api-key permission but not grant-role permission for all roles on an API key. However, I don't see a use case for this since, unlike API key creation, deletion in practice is tightly controlled and limited only to Emo administrators. It seems like a bad practice to delegate this particular permission to non-trusted administrators.

TL;DR: I have no objection to adding this check, it just seemed redundant since any user with delete permission would be an administrator.

[billkalter]

@sujithvaddi Addressed feedback

[billkalter]

jenkins retest this please

[sujithvaddi]

@billkalter Looks good. 👍