Add manage-application-privileges conditional cluster privilege #32116
Conversation
This includes support in the Roles API and for storing in the security index.
tvernum
added
review
:Security/Authorization
|
Pinging @elastic/es-security |
| @@ -116,6 +116,7 @@ private Builder(RoleDescriptor rd, @Nullable FieldPermissionsCache fieldPermissi | |||
| applicationPrivs.add(convertApplicationPrivilege(rd.getName(), i, applicationPrivileges[i])); | |||
| } | |||
|
|
|||
|
|
|||
| } | ||
|
|
||
| /** | ||
| * Read a list of privileges from the parser. The parse should be positioned at the |
| public static List<ConditionalClusterPrivilege> parse(XContentParser parser) throws IOException { | ||
| List<ConditionalClusterPrivilege> privileges = new ArrayList<>(); | ||
|
|
||
| if (parser.currentToken() == null) { |
There was a problem hiding this comment.
this goes against the javadocs for the method and is lenient. currentToken should be start object here
| public static class ManageApplicationPrivileges implements ConditionalClusterPrivilege { | ||
|
|
||
| private static final ClusterPrivilege PRIVILEGE = ClusterPrivilege.get( | ||
| Collections.singleton("cluster:admin/xpack/security/privilege/*") |
There was a problem hiding this comment.
seeing this here make me think we need to add application in the action names. What do you think?
There was a problem hiding this comment.
I think this is a bigger question.
At the moment the classes, endpoints and action-names all simply refer to "privilege". When I wrote them, I had the thought that perhaps in the future we might include support for cluster & index privileges in the GET API.
I'm less convinced that's a great idea now - but if we want to rename the actions, then we should probably rename it all.
There was a problem hiding this comment.
it looks like application is not required so I am ok either way. I will leave the naming up to you
|
|
||
| @Override | ||
| public boolean test(TransportRequest request) { | ||
| if (request instanceof GetPrivilegesRequest) { |
There was a problem hiding this comment.
I wonder if we should have a interface like UserRequest that returns a String[] of the application names? This simplifies the code here a bit
| if (parser.currentToken() == XContentParser.Token.START_OBJECT) { | ||
| parser.nextToken(); | ||
| } | ||
| expectedToken(parser.currentToken(), parser, XContentParser.Token.FIELD_NAME); |
There was a problem hiding this comment.
I wonder if we can be stricter here around the state of the parser?
| final PutRoleRequest original = buildRandomRequest(); | ||
|
|
||
| final BytesStreamOutput out = new BytesStreamOutput(); | ||
| final Version version = randomFrom(Version.V_5_6_10, Version.V_6_0_0, Version.V_6_1_2, Version.V_6_2_4, Version.V_6_3_0); |
There was a problem hiding this comment.
I think VersionUtils#randomVersionBetween is a better choice here
| @@ -336,6 +336,7 @@ public void authorize(Authentication authentication, String action, TransportReq | |||
| auditTrail.accessGranted(authentication, action, request, permission.names()); | |||
| } | |||
|
|
|||
|
|
|||
| import java.util.Collection; | ||
|
|
||
| /** | ||
| * Interface implemented by all Requests that managed application privileges |
| @@ -53,6 +53,7 @@ public void testGenerateAndParseXContent() throws Exception { | |||
|
|
|||
| final byte[] bytes = out.toByteArray(); | |||
| try (XContentParser parser = xContent.createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, bytes)) { | |||
| assertThat( parser.nextToken(), equalTo(XContentParser.Token.START_OBJECT)); | |||
There was a problem hiding this comment.
nit: remove space before parser
|
run gradle build tests |
This includes support in the Roles API and for storing in the security index.
Traditional, action-name cluster privileges are still described by a
cluster: []element in the JSON, while conditional privileges are described in a
policy: {}element (the name is subject to change).
For the roles API, and builtin roles providers (native + file) the only supported
conditional privilege is ManageApplicationPrivileges represented in JSON as:
which restricts the use of the Get/Put/Delete Privileges actions to those that act upon the specified application names (
"my-app","app-*")