Skip to content

Commit da9a00a

Browse files
gwbrowngwbrown
committed
More fixes to issues raised in tests and fixes to tests
1 parent 762a84f commit da9a00a

File tree

5 files changed

+54
-65
lines changed

5 files changed

+54
-65
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Group.java‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,8 @@ public IndicesPermission.AuthorizedComponents check(String name, IndexAbstractio
131131
&& group.hasMappingUpdateBwcPermissions
132132
&& resource != null
133133
&& resource.getParentDataStream() == null
134-
&& resource.getType() != IndexAbstraction.Type.DATA_STREAM) { // ATHE does this grant too often?
134+
&& resource.getType() != IndexAbstraction.Type.DATA_STREAM
135+
&& group.indexNameMatcher.test(name)) {
135136
boolean alreadyLogged = deprecationLogEmitted.getAndSet(true);
136137
if (alreadyLogged == false) {
137138
for (String privilegeName : group.privilege.name()) {
@@ -239,16 +240,12 @@ Automaton getIndexMatcherAutomaton() {
239240
return indexNameAutomaton.get();
240241
}
241242

242-
boolean noFieldLevelSecurity() {
243+
boolean allowsTotalDataIndexAccess() {
243244
return allowRestrictedIndices
244245
&& indexNameMatcher.isTotal()
245-
&& privilege.name().contains(IndexPrivilege.ALL)
246-
&& query == null
247-
&& false == fieldPermissions.hasFieldLevelSecurity();
248-
}
249-
250-
boolean noRestrictions() {
251-
return noFieldLevelSecurity() && hasReadFailuresPrivilege;
246+
&& privilege.name().contains("all") // ATHE: probably make this into a constant
247+
&& hasReadFailuresPrivilege
248+
&& query == null;
252249
}
253250

254251
@Override

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ public IndicesPermission build() {
147147
private IndicesPermission(RestrictedIndices restrictedIndices, Group[] groups) {
148148
this.restrictedIndices = restrictedIndices;
149149
this.groups = groups;
150-
this.hasFieldOrDocumentLevelSecurity = Arrays.stream(groups).noneMatch(Group::noFieldLevelSecurity)
150+
this.hasFieldOrDocumentLevelSecurity = Arrays.stream(groups).noneMatch(Group::allowsTotalDataIndexAccess)
151151
&& Arrays.stream(groups).anyMatch(g -> g.hasQuery() || g.getFieldPermissions().hasFieldLevelSecurity());
152152
}
153153

@@ -432,13 +432,6 @@ public IndicesAccessControl authorize(
432432
Map<String, IndexAbstraction> lookup,
433433
FieldPermissionsCache fieldPermissionsCache
434434
) {
435-
// Short circuit if the indicesPermission allows all access to every index
436-
for (Group group : groups) {
437-
if (group.noRestrictions()) {
438-
return IndicesAccessControl.allowAll();
439-
}
440-
}
441-
442435
final Map<String, IndexResource> resources = Maps.newMapWithExpectedSize(requestedIndicesOrAliases.size());
443436
int totalResourceCount = 0;
444437

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -455,12 +455,9 @@ static String getPutMappingIndexOrAlias(PutMappingRequest request, Predicate<Str
455455
for (var aliasMd : foundAliases.values()) {
456456
String aliasName = aliasMd.alias();
457457
IndexAbstraction aliasAbstraction = metadata.getIndicesLookup().get(aliasName);
458-
List<Index> indices = aliasAbstraction.getIndices();
459-
if (indices.size() == 1 || Boolean.TRUE.equals(aliasMd.writeIndex())) {
460-
return aliasName;
461-
} else {
462-
assert aliasAbstraction.getType() == IndexAbstraction.Type.ALIAS;
463-
Index writeIndex = aliasAbstraction.getWriteIndex();
458+
assert aliasAbstraction.getType() == IndexAbstraction.Type.ALIAS;
459+
Index writeIndex = aliasAbstraction.getWriteIndex();
460+
if (writeIndex != null) {
464461
if (concreteIndexName.equals(writeIndex.getName()) && isAuthorized.test(aliasName)) {
465462
return aliasName;
466463
}

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java‎

Lines changed: 41 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import org.elasticsearch.cluster.metadata.Metadata;
4343
import org.elasticsearch.cluster.service.ClusterService;
4444
import org.elasticsearch.common.Strings;
45+
import org.elasticsearch.common.UUIDs;
4546
import org.elasticsearch.common.regex.Regex;
4647
import org.elasticsearch.common.settings.ClusterSettings;
4748
import org.elasticsearch.common.settings.Settings;
@@ -341,7 +342,7 @@ public void setup() {
341342
new RoleDescriptor(
342343
"data_stream_test3",
343344
null,
344-
new IndicesPrivileges[] { IndicesPrivileges.builder().indices("logs*").privileges("all").build() },
345+
new IndicesPrivileges[] { IndicesPrivileges.builder().indices("logs*").privileges("all", "read_failures").build() },
345346
null
346347
)
347348
);
@@ -1929,40 +1930,45 @@ public void testAliasDateMathExpressionNotSupported() {
19291930
assertThat(request.aliases(), arrayContainingInAnyOrder("<datetime-{now/M}>"));
19301931
}
19311932

1932-
// public void testDynamicPutMappingRequestFromAlias() {
1933-
// PutMappingRequest request = new PutMappingRequest(Strings.EMPTY_ARRAY).setConcreteIndex(new Index("foofoo", UUIDs.base64UUID()));
1934-
// User user = new User("alias-writer", "alias_read_write");
1935-
// AuthorizedIndices authorizedIndices = buildAuthorizedIndices(user, TransportPutMappingAction.TYPE.name());
1936-
//
1937-
// String putMappingIndexOrAlias = IndicesAndAliasesResolver.getPutMappingIndexOrAlias(request, authorizedIndices::check, metadata);
1938-
// assertEquals("barbaz", putMappingIndexOrAlias);
1939-
//
1940-
// // multiple indices map to an alias so we can only return the concrete index
1941-
// final String index = randomFrom("foo", "foobar");
1942-
// request = new PutMappingRequest(Strings.EMPTY_ARRAY).setConcreteIndex(new Index(index, UUIDs.base64UUID()));
1943-
// putMappingIndexOrAlias = IndicesAndAliasesResolver.getPutMappingIndexOrAlias(request, authorizedIndices::check, metadata);
1944-
// assertEquals(index, putMappingIndexOrAlias);
1945-
// }
1946-
//
1947-
// public void testWhenAliasToMultipleIndicesAndUserIsAuthorizedUsingAliasReturnsAliasNameForDynamicPutMappingRequestOnWriteIndex() {
1948-
// String index = "logs-00003"; // write index
1949-
// PutMappingRequest request = new PutMappingRequest(Strings.EMPTY_ARRAY).setConcreteIndex(new Index(index, UUIDs.base64UUID()));
1950-
// assert metadata.getIndicesLookup().get("logs-alias").getIndices().size() == 3;
1951-
// String putMappingIndexOrAlias = IndicesAndAliasesResolver.getPutMappingIndexOrAlias(request, "logs-alias"::equals, metadata);
1952-
// String message = "user is authorized to access `logs-alias` and the put mapping request is for a write index"
1953-
// + "so this should have returned the alias name";
1954-
// assertEquals(message, "logs-alias", putMappingIndexOrAlias);
1955-
// }
1956-
1957-
// public void testWhenAliasToMultipleIndicesAndUserIsAuthorizedUsingAliasReturnsIndexNameForDynamicPutMappingRequestOnReadIndex() {
1958-
// String index = "logs-00002"; // read index
1959-
// PutMappingRequest request = new PutMappingRequest(Strings.EMPTY_ARRAY).setConcreteIndex(new Index(index, UUIDs.base64UUID()));
1960-
// assert metadata.getIndicesLookup().get("logs-alias").getIndices().size() == 3;
1961-
// String putMappingIndexOrAlias = IndicesAndAliasesResolver.getPutMappingIndexOrAlias(request, "logs-alias"::equals, metadata);
1962-
// String message = "user is authorized to access `logs-alias` and the put mapping request is for a read index"
1963-
// + "so this should have returned the concrete index as fallback";
1964-
// assertEquals(message, index, putMappingIndexOrAlias);
1965-
// }
1933+
public void testDynamicPutMappingRequestFromAlias() {
1934+
PutMappingRequest request = new PutMappingRequest(Strings.EMPTY_ARRAY).setConcreteIndex(new Index("foofoo", UUIDs.base64UUID()));
1935+
User user = new User("alias-writer", "alias_read_write");
1936+
AuthorizedIndices authorizedIndices = buildAuthorizedIndices(user, TransportPutMappingAction.TYPE.name());
1937+
1938+
String putMappingIndexOrAlias = IndicesAndAliasesResolver.getPutMappingIndexOrAlias(request, authorizedIndices::check, metadata);
1939+
assertEquals("barbaz", putMappingIndexOrAlias);
1940+
1941+
// multiple indices map to an alias so we can only return the concrete index
1942+
final String index = randomFrom("foo", "foobar");
1943+
request = new PutMappingRequest(Strings.EMPTY_ARRAY).setConcreteIndex(new Index(index, UUIDs.base64UUID()));
1944+
putMappingIndexOrAlias = IndicesAndAliasesResolver.getPutMappingIndexOrAlias(request, authorizedIndices::check, metadata);
1945+
assertEquals(index, putMappingIndexOrAlias);
1946+
assertWarnings(
1947+
"the index privilege [write] allowed the update mapping action [indices:admin/mapping/put] on "
1948+
+ "index [barbaz], this privilege will not permit mapping updates in the next major release - users who require access to "
1949+
+ "update mappings must be granted explicit privileges"
1950+
);
1951+
}
1952+
1953+
public void testWhenAliasToMultipleIndicesAndUserIsAuthorizedUsingAliasReturnsAliasNameForDynamicPutMappingRequestOnWriteIndex() {
1954+
String index = "logs-00003"; // write index
1955+
PutMappingRequest request = new PutMappingRequest(Strings.EMPTY_ARRAY).setConcreteIndex(new Index(index, UUIDs.base64UUID()));
1956+
assert metadata.getIndicesLookup().get("logs-alias").getIndices().size() == 3;
1957+
String putMappingIndexOrAlias = IndicesAndAliasesResolver.getPutMappingIndexOrAlias(request, "logs-alias"::equals, metadata);
1958+
String message = "user is authorized to access `logs-alias` and the put mapping request is for a write index"
1959+
+ "so this should have returned the alias name";
1960+
assertEquals(message, "logs-alias", putMappingIndexOrAlias);
1961+
}
1962+
1963+
public void testWhenAliasToMultipleIndicesAndUserIsAuthorizedUsingAliasReturnsIndexNameForDynamicPutMappingRequestOnReadIndex() {
1964+
String index = "logs-00002"; // read index
1965+
PutMappingRequest request = new PutMappingRequest(Strings.EMPTY_ARRAY).setConcreteIndex(new Index(index, UUIDs.base64UUID()));
1966+
assert metadata.getIndicesLookup().get("logs-alias").getIndices().size() == 3;
1967+
String putMappingIndexOrAlias = IndicesAndAliasesResolver.getPutMappingIndexOrAlias(request, "logs-alias"::equals, metadata);
1968+
String message = "user is authorized to access `logs-alias` and the put mapping request is for a read index"
1969+
+ "so this should have returned the concrete index as fallback";
1970+
assertEquals(message, index, putMappingIndexOrAlias);
1971+
}
19661972

19671973
public void testHiddenIndicesResolution() {
19681974
SearchRequest searchRequest = new SearchRequest();

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -582,11 +582,6 @@ public void testAuthorizationForMappingUpdates() {
582582
+ "] on "
583583
+ "index [test_write1], this privilege will not permit mapping updates in the next major release - "
584584
+ "users who require access to update mappings must be granted explicit privileges",
585-
"the index privilege [write] allowed the update mapping action ["
586-
+ TransportPutMappingAction.TYPE.name()
587-
+ "] on "
588-
+ "index [test1], this privilege will not permit mapping updates in the next major release - "
589-
+ "users who require access to update mappings must be granted explicit privileges",
590585
"the index privilege [write] allowed the update mapping action ["
591586
+ TransportPutMappingAction.TYPE.name()
592587
+ "] on "
@@ -679,9 +674,10 @@ public void testIndicesPermissionHasFieldOrDocumentLevelSecurity() {
679674
).build();
680675
assertThat(indicesPermission2.hasFieldOrDocumentLevelSecurity(), is(false));
681676

682-
// IsTotal means NO DLS/FLS even when there is another group that has DLS/FLS
677+
// allowsTotalDataIndexAccess means NO DLS/FLS even when there is another group that has DLS/FLS
678+
// failure store indices are not considered as
683679
final IndicesPermission indicesPermission3 = new IndicesPermission.Builder(RESTRICTED_INDICES).addGroup(
684-
IndexPrivilege.ALL,
680+
IndexPrivilege.get(Set.of("all", "read_failures")),
685681
FieldPermissions.DEFAULT,
686682
null,
687683
true,

0 commit comments

Comments
 (0)