Support multi-intersection for FieldPermissions #91169
Conversation
ywangd
added
>refactoring
:Security/Authorization
|
Pinging @elastic/es-security (Team:Security) |
| private final FieldPermissionsDefinition fieldPermissionsDefinition; | ||
| @Nullable | ||
| private final FieldPermissionsDefinition limitedByFieldPermissionsDefinition; | ||
| private final List<FieldPermissionsDefinition> fieldPermissionsDefinitions; |
There was a problem hiding this comment.
This is the critical change of this PR: replacing two hard coded variables with a List (similar to that of #91151). Everything else revolves around it.
...e/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java
Outdated
Show resolved
Hide resolved
| FieldPermissions getFieldPermissions(Collection<FieldPermissions> fieldPermissionsCollection) { | ||
| FieldPermissions union(Collection<FieldPermissions> fieldPermissionsCollection) { |
There was a problem hiding this comment.
Rename this method because it should only be used for union field permissions within the same role. It must not be used for multiple limiting (intersecting) roles.
| // TODO: Use FieldPermissionsDefinition instead of FieldPermissions. The former is a better counterpart to query | ||
| private final FieldPermissions fieldPermissions; | ||
| private final Set<BytesReference> query; |
There was a problem hiding this comment.
The TODO here is to note down that using FieldPermissions here is inconsistent when comparing to the query field (Set<BytesReference> instead of DocumentPermissions). At this level, it is better to use FieldPermissionsDefinition and only build FieldPermissions when computing IndicesAccessControl.
| public FieldPermissions() { | ||
| private FieldPermissions() { | ||
| this(new FieldPermissionsDefinition(null, null), Automatons.MATCH_ALL); | ||
| } |
There was a problem hiding this comment.
Make this constructor private since it is only really useful for building the static FieldPermissions.DEFAULT variable. Its usages have been replaced by directly using the variable. This change touched a few files.
...e/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java
Outdated
Show resolved
Hide resolved
.../test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java
Outdated
Show resolved
Hide resolved
|
|
||
| assertThat(fieldPermissions.grantsAccessTo("f3" + randomAlphaOfLengthBetween(1, 10)), is(true)); | ||
| assertThat(fieldPermissions.grantsAccessTo("f4" + randomAlphaOfLengthBetween(1, 10)), is(true)); | ||
| } |
There was a problem hiding this comment.
Can you help me understand why the following passes
fieldPermissions = FieldPermissions.DEFAULT;
fieldPermissions = fieldPermissions.limitFieldPermissions(
new FieldPermissions(fieldPermissionDef(new String[] { "f1", "f2" }, new String[] { "" }))
);
assertThat(fieldPermissions.grantsAccessTo("f1"), is(true));
assertThat(fieldPermissions.grantsAccessTo("f2"), is(true));
but the following does not
fieldPermissions = FieldPermissions.DEFAULT;
fieldPermissions = fieldPermissions.limitFieldPermissions(
new FieldPermissions(fieldPermissionDef(new String[] { "f1" }, new String[] { "" }))
);
fieldPermissions = fieldPermissions.limitFieldPermissions(
new FieldPermissions(fieldPermissionDef(new String[] { "f2" }, new String[] { "" }))
);
assertThat(fieldPermissions.grantsAccessTo("f1"), is(true));
assertThat(fieldPermissions.grantsAccessTo("f2"), is(true));
Shouldn't those two cases behave identical since we are saying grant access to "f1" and grant access to "f2" ?
There was a problem hiding this comment.
The 1st test case is equivalent to a SimpleRole (e.g. for realm users), in role descriptor form it is like
{
"field_security": {
"grant": [ "f1", "f2" ],
"except": [ "" ]
}
}
It is pretty clear that the user is granted for access both f1 and f2.
The 2nd test case is equivalent to a LimitedRole (for API keys), in role descriptor form it is like
{
"key_role": {
"field_security": {
"grant": [ "f1" ],
"except": [ "" ]
}
},
"limited_by": {
"field_security": {
"grant": [ "f2" ],
"except": [ "" ]
}
}
}
Even though the key's role descriptor says it can access filed f1, it is not allowed by the limiting user's role (only allow f2). Similarly, f2 is not accessible because the key's role does not allow it.
For any field to be accessible, it must be allowed by both key role and the limiting user role.
There was a problem hiding this comment.
Thanks, presented that way, makes perfect sense !
|
|
||
| assertThat(fieldPermissions.grantsAccessTo("f3" + randomAlphaOfLengthBetween(1, 10)), is(true)); | ||
| assertThat(fieldPermissions.grantsAccessTo("f4" + randomAlphaOfLengthBetween(1, 10)), is(true)); | ||
| } |
There was a problem hiding this comment.
Thanks, presented that way, makes perfect sense !
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java
Outdated
Show resolved
Hide resolved
…ecurity/authz/RBACEngine.java Co-authored-by: Jake Landis <jake.landis@elastic.co>
|
@elasticmachine update branch |
* main: (1300 commits) update c2id/c2id-server-demo docker image to support ARM (elastic#91144) Allow legacy index settings on legacy indices (elastic#90264) Skip prevoting if single-node discovery (elastic#91255) Chunked encoding for snapshot status API (elastic#90801) Allow different decay values depending on the score function (elastic#91195) Fix handling indexed envelopes crossing the dateline in mvt API (elastic#91105) Ensure cleanups succeed in JoinValidationService (elastic#90601) Add overflow behaviour test for RecyclerBytesStreamOutput (elastic#90638) More actionable error for ancient indices (elastic#91243) Fix APM configuration file delete (elastic#91058) Clean up handshake test class (elastic#90966) Improve H3#hexRing logic and add H3#areNeighborCells method (elastic#91140) Restrict direct use of `ApplicationPrivilege` constructor (elastic#91176) [ML] Allow NLP truncate option to be updated when span is set (elastic#91224) Support multi-intersection for FieldPermissions (elastic#91169) Support intersecting multi-sets of queries with DocumentPermissions (elastic#91151) Ensure TermsEnum action works correctly with API keys (elastic#91170) Fix NPE in auditing authenticationSuccess for non-existing run-as user (elastic#91171) Ensure PKI's delegated_by_realm metadata respect run-as (elastic#91173) [ML] Update API documentation for anomaly score explanation (elastic#91177) ... # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/TransportRollupIndexerAction.java # x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java
This PR is the 2nd half of updating DocumentPermissions and FieldPermissions to support multi-level of limiting similar to LimitedRole (since #81403). Instead of hard coding fieldsDefinition and limitedByFieldsDefinition, this PR replaces them with a list of fieldsDefinitions which can accomodate multiple of them (more than 2).
Relates: #91151