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

Changes from 1 commit
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
10 changes: 10 additions & 0 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 @@ -353,8 +353,18 @@ private void migrateApiKey(Subject subject, ImmutableMultimap<String, String> pa
}

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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:?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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