Handle has privileges API response for `LimitedRole` #37831

bizybot · 2019-01-24T17:36:36Z

With the introduction of LimitedRole for API keys, the effective
permissions are constrained to the permissions defined by the
limit by role.
This impacts the way we provide a response for Has Privileges API.
This change introduces a few helper methods on Role object which
can be used to get resources privileges (index/application).
The methods are overridden in LimitedRole to take an intersection
of the privileges and return the result.

The LimitedRole changes also impact Get privileges API but we
would not be handling that case. The API is primarily aimed at
internal(Kibana) usage, the API is not supposed to be used for
authorization decisions and has been documented as such.

With introduction of `LimitedRole` for API keys, the effective permissions are constrained to the permissions defined by the limit by role. This impacts the way we provide response for Has Privileges API. This change introduces few helper methods on `Role` object which can be used to get resources privileges (index / application). The methods are overridden in `LimitedRole` to take an intersection of the privileges and return the result. The `LimitedRole` changes also impact Get privileges API but we would not be handling that case. The API is primarily aimed at internal(Kibana) usage, the API is not supposed to be used for authorization decisions and has been documented as such.

elasticmachine · 2019-01-24T17:36:38Z

Pinging @elastic/es-security

…es-api

tvernum

I left a bunch of mostly style comments. Overall I think this is the right approach.

tvernum · 2019-01-25T00:52:41Z

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/ResourcesPrivileges.java

+/**
+ * A generic structure to encapsulate resources to {@link ResourcePrivileges}.
+ */
+public final class ResourcesPrivileges {


I know we do it in a few other places, but I find this name of ResourcePrivileges and ResourcesPrivileges to be confusing and easy to mistake. It took me a several reads of the new Javadoc that refers to both in classes in the same sentence (example).

When the plural is at the end, it's not so bad, but here we need to pluralise in the middle, and I (personally) think that's too hard to notice. I'd rather we rename this class, even if it's just ResourcePrivilegesMap or something.

Yes, after going through your comment I did feel taxing to understand, so thank you for pushing the naming. I have addressed this.

tvernum · 2019-01-25T00:55:39Z

.../main/java/org/elasticsearch/xpack/core/security/authz/permission/ApplicationPermission.java

+     * @param storedPrivileges stored {@link ApplicationPrivilegeDescriptor} for the application
+     * @return an instance of {@link ResourcesPrivileges}
+     */
+    public ResourcesPrivileges getResourcePrivileges(final String applicationName, List<String> forResources,


I think calling this a get method underplays what it does, and assumes that caller knows what "ResourcePrivileges" is supposed to represent.

checkResourcePrivileges or checkResourceAccess feel better to me.

+1, changed the method to reflect what it does. Thank you.

tvernum · 2019-01-25T01:01:33Z

.../main/java/org/elasticsearch/xpack/core/security/authz/permission/ApplicationPermission.java

+     * @param forResources list of application resources
+     * @param forPrivilegeNames list of application privileges
+     * @param storedPrivileges stored {@link ApplicationPrivilegeDescriptor} for the application
+     * @return an instance of {@link ResourcesPrivileges}


(Begin rant)

I don't feel as though these @param annotations really add much. They're more or less repeating what is obvious from the type and name.
The most obvious example:

@param applicationName application name

but even

@param storedPrivileges stored {@link ApplicationPrivilegeDescriptor} for the application

We know it's stored because the parameter is named stored... and we know they're ApplicationPrivilegeDescriptor object from the parameter type. The javadoc should be adding additional details that aren't implied by the name/type.

I think it's good to document some of these, particularly storedPrivileges because it isn't obvious (on the face of it) why a caller needs to pass in a bunch of privilege objects to this method, but the doc above doesn't explain that.

(End rant: It's not specifically this PR, it comes up in other places, I just happen to have some spare time today so I'm taking the opportunity to express myself)

Sorry for this, I think I was not good with the documentation here. I have updated the documentation with more information to express the intent of the parameters and explain how it is being used. Thank you.

tvernum · 2019-01-25T01:06:00Z

.../main/java/org/elasticsearch/xpack/core/security/authz/permission/ApplicationPermission.java

+                        + applicationName;
+                assert checkPrivilege.name().equals(nameSet) : "Privilege " + checkPrivilege + " should have name " + nameSet;
+
+                ResourcePrivileges.Builder builder = ResourcePrivileges.builder().setResource(checkResource);


Suggestion: I think this would read better (and be a safer API) if the resource name was a pararmeter to the builder method rather than a set call.

Yes, I did not realize it while writing the code but your suggestion is spot on. Thank you.

tvernum · 2019-01-25T01:09:04Z

.../main/java/org/elasticsearch/xpack/core/security/authz/permission/ApplicationPermission.java

+                    builder.addPrivilege(checkPrivilegeName, Boolean.FALSE);
+                    allowAll = false;
+                }
+                result.compute(checkResource, (k, v) -> (v == null) ? builder.build() : builder.addPrivileges(v.getPrivileges()).build());


I find this a little weird.
I guess it's probably come about because this was refactored from existing code, and the changed, but it seems a bit strange to keep putting stuff in and out of the map like this when we have a Builder class that can have stuff added to it.
This method causes a lot of object churn that isn't really necessary.

Did you consider having the map values be Builder object instead, and then just do a final .build() pass at the end of the method?

I have addressed this with Builder object as suggested here and other places, Thank you.

tvernum · 2019-01-25T01:57:29Z

...n/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRole.java

+                    privileges.put(privilege, Boolean.TRUE);
+                } else {
+                    privileges.put(privilege, Boolean.FALSE);
+                }


I would do this with an && as well, but I'm not fussed either way.

Addressed this, Thank you.

tvernum · 2019-01-25T01:59:19Z

...src/main/java/org/elasticsearch/xpack/core/security/authz/permission/ResourcePrivileges.java

+    }
+
+    public boolean isAllowed(String privilege) {
+        return (privileges.get(privilege)) != null ? privileges.get(privilege) : false;


Rather than doing 2 gets on the map you could do:

Suggested change

return (privileges.get(privilege)) != null ? privileges.get(privilege) : false;

return Boolean.TRUE.equals(privileges.get(privilege));

Addressed this, Thank you.

tvernum · 2019-01-25T02:00:30Z

...src/main/java/org/elasticsearch/xpack/core/security/authz/permission/ResourcePrivileges.java

+    }
+
+    public static class Builder {
+        private String resource;


Per earlier comment, I'd make the resource final and require it in the constructor.

Addressed this, Thank you.

tvernum · 2019-01-25T02:06:13Z

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java

+     * @param forPrivileges list of index privileges
+     * @return a map of resource to {@link ResourcePrivileges}
+     */
+    public ResourcesPrivileges getResourcePrivileges(List<String> forIndexPatterns, boolean allowRestrictedIndices,


If this method is going to exist, then it needs index in its name somewhere, but I don't think it's really necessary is it?

changed the method name to checkIndicesPrivileges, Thank you.

tvernum · 2019-01-25T02:06:53Z

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java

+     * @param storedPrivileges stored {@link ApplicationPrivilegeDescriptor} for the application
+     * @return an instance of {@link ResourcesPrivileges}
+     */
+    public ResourcesPrivileges getResourcePrivileges(final String applicationName, List<String> forResources,


Needs application in its name.

changed method name to checkApplicationResourcePrivileges, Thank you.

- Correct the behavior for creating the has privileges API response. - Add tests - Add documentation with more explanation - Use of proper builders

…es-api

jaymode

I left a few comments, mostly minor. Otherwise it LGTM, but please ensure @tvernum approves prior to merging.

jaymode · 2019-01-25T21:16:51Z

.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/ClusterPermission.java

@@ -33,6 +34,10 @@ public ClusterPrivilege privilege() {

    public abstract boolean check(String action, TransportRequest request);

+    public boolean check(ClusterPrivilege clusterPrivilege) {


I do not think this should be named check. The method name is ambiguous

Changed it to grants, Thanks.

jaymode · 2019-01-25T21:19:06Z

...n/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRole.java

@@ -44,18 +47,18 @@ public IndicesAccessControl authorize(String action, Set<String> requestedIndice
    }

    /**
-     * @return A predicate that will match all the indices that this role and
-     * the scoped by role has the privilege for executing the given action on.
+     * @return A predicate that will match all the indices that this role and the scoped by role has the privilege for executing the given


this still mentions scoped by; I think we should change the wording to be more along the lines of limiting

Done, Thank you.

jaymode · 2019-01-25T21:21:18Z

.../main/java/org/elasticsearch/xpack/core/security/authz/permission/ResourcePrivilegesMap.java

+
+    public static class Builder {
+        private boolean allowAll = true;
+        private Map<String, ResourcePrivileges.Builder> resourceToResourcePrivilegesBuilder = new LinkedHashMap<>();


make this final

Done, Thank you.

jaymode · 2019-01-25T21:21:31Z

...src/main/java/org/elasticsearch/xpack/core/security/authz/permission/ResourcePrivileges.java

+
+    public static class Builder {
+        private final String resource;
+        private Map<String, Boolean> privileges = new HashMap<>();


make this final

Done, Thank you.

bizybot · 2019-01-29T04:14:47Z

@elasticmachine -- run elasticsearch-ci/oss-distro-docs

…es-api

bizybot · 2019-01-29T09:50:41Z

@elasticmachine
run elasticsearch-ci/1
run elasticsearch-ci/default-distro

…es-api

bizybot added >non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Jan 24, 2019

bizybot requested review from tvernum and jaymode January 24, 2019 17:36

Yogesh Gaikwad added 2 commits January 25, 2019 11:53

Merge branch 'security_api_keys' into authz-limited-role-has-privileg…

69321a8

…es-api

correct the logic for application privileges

f1a44e1

tvernum reviewed Jan 25, 2019

View reviewed changes

Yogesh Gaikwad added 2 commits January 25, 2019 22:49

Address review comments

7d96abd

- Correct the behavior for creating the has privileges API response. - Add tests - Add documentation with more explanation - Use of proper builders

Merge branch 'security_api_keys' into authz-limited-role-has-privileg…

8ff13d6

…es-api

jaymode approved these changes Jan 25, 2019

View reviewed changes

Address review comments

ff0e073

tvernum approved these changes Jan 29, 2019

View reviewed changes

Merge branch 'security_api_keys' into authz-limited-role-has-privileg…

94b08b1

…es-api

Yogesh Gaikwad added 3 commits January 30, 2019 14:03

Merge branch 'security_api_keys' into authz-limited-role-has-privileg…

a07ab87

…es-api

Merge branch 'security_api_keys' into authz-limited-role-has-privileg…

547d93b

…es-api

Merge branch 'security_api_keys' into authz-limited-role-has-privileg…

1e7844c

…es-api

bizybot merged commit 382d6cb into elastic:security_api_keys Jan 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle has privileges API response for `LimitedRole` #37831

Handle has privileges API response for `LimitedRole` #37831

bizybot commented Jan 24, 2019

elasticmachine commented Jan 24, 2019

tvernum left a comment

tvernum Jan 25, 2019

bizybot Jan 25, 2019

tvernum Jan 25, 2019

bizybot Jan 25, 2019

tvernum Jan 25, 2019

bizybot Jan 25, 2019

tvernum Jan 25, 2019

bizybot Jan 25, 2019

tvernum Jan 25, 2019

bizybot Jan 25, 2019

tvernum Jan 25, 2019

bizybot Jan 25, 2019

tvernum Jan 25, 2019

bizybot Jan 25, 2019

tvernum Jan 25, 2019

bizybot Jan 25, 2019

tvernum Jan 25, 2019

bizybot Jan 25, 2019

tvernum Jan 25, 2019

bizybot Jan 25, 2019

jaymode left a comment

jaymode Jan 25, 2019

bizybot Jan 26, 2019

jaymode Jan 25, 2019

bizybot Jan 26, 2019

jaymode Jan 25, 2019

bizybot Jan 26, 2019

jaymode Jan 25, 2019

bizybot Jan 26, 2019

bizybot commented Jan 29, 2019

bizybot commented Jan 29, 2019

	return (privileges.get(privilege)) != null ? privileges.get(privilege) : false;
	return Boolean.TRUE.equals(privileges.get(privilege));

		@@ -33,6 +34,10 @@ public ClusterPrivilege privilege() {

		public abstract boolean check(String action, TransportRequest request);

		public boolean check(ClusterPrivilege clusterPrivilege) {

Handle has privileges API response for LimitedRole #37831

Handle has privileges API response for LimitedRole #37831

Conversation

bizybot commented Jan 24, 2019

elasticmachine commented Jan 24, 2019

tvernum 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

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

jaymode 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

bizybot commented Jan 29, 2019

bizybot commented Jan 29, 2019

Handle has privileges API response for `LimitedRole` #37831

Handle has privileges API response for `LimitedRole` #37831