Skip to content

Commit

Permalink
Fix issue with Get mappings on a Closed index (#4685)
Browse files Browse the repository at this point in the history 
Signed-off-by: Craig Perkins <cwperx@amazon.com>
  • Loading branch information
@cwperks
cwperks authored Oct 4, 2024
1 parent 5cb4dd2 commit 99457b1
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 11 deletions.
36 changes: 36 additions & 0 deletions src/integrationTest/java/org/opensearch/security/FlsAndFieldMaskingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import org.opensearch.test.framework.cluster.TestRestClient;
import org.opensearch.test.framework.log.LogsRule;

import static org.apache.http.HttpStatus.SC_OK;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -230,6 +231,12 @@ public class FlsAndFieldMaskingTests {
"cluster_composite_ops_ro"
).indexPermissions("read").fls(String.format("~%s", FIELD_TITLE)).on(FIRST_INDEX_NAME);

static final TestSecurityConfig.Role ROLE_NO_FIELD_TITLE_WILDCARD_INDEX_FLS = new TestSecurityConfig.Role("example_exclusive_fls")
.clusterPermissions("cluster_composite_ops_ro")
.indexPermissions("read", "indices:admin/mappings/get")
.fls(String.format("~%s", FIELD_TITLE))
.on("*");

static final TestSecurityConfig.Role ROLE_ONLY_FIELD_TITLE_MASKED = new TestSecurityConfig.Role("example_mask").clusterPermissions(
"cluster_composite_ops_ro"
).indexPermissions("read").maskedFields(FIELD_TITLE.concat("::/(?<=.{1})./::").concat(MASK_VALUE)).on(FIRST_INDEX_NAME);
Expand All @@ -248,6 +255,12 @@ public class FlsAndFieldMaskingTests {
ROLE_NO_FIELD_TITLE_FLS
);

/**
* Example user with fls filter in which the user can see every field but the {@link Song#FIELD_TITLE} field.
*/
static final TestSecurityConfig.User USER_NO_FIELD_TITLE_WILDCARD_INDEX_FLS = new TestSecurityConfig.User("exclusive_wildcard_fls_user")
.roles(ROLE_NO_FIELD_TITLE_WILDCARD_INDEX_FLS);

/**
* Example user in which {@link Song#FIELD_TITLE} field is masked.
*/
Expand Down Expand Up @@ -305,6 +318,7 @@ public class FlsAndFieldMaskingTests {
ALL_INDICES_STARS_LESS_THAN_ZERO_READER,
TWINS_FIRST_ARTIST_READER,
USER_ONLY_FIELD_TITLE_FLS,
USER_NO_FIELD_TITLE_WILDCARD_INDEX_FLS,
USER_NO_FIELD_TITLE_FLS,
USER_ONLY_FIELD_TITLE_MASKED,
USER_BOTH_ONLY_AND_NO_FIELD_TITLE_FLS,
Expand Down Expand Up @@ -1778,4 +1792,26 @@ public void testFlsOnAClosedAndReopenedIndex() throws IOException {
}
}

@SuppressWarnings("unchecked")
@Test
public void testGetMappingsOnAClosedIndexWithFlsRestrictions() throws IOException {
String indexName = "fls_excludes_mappings";
List<String> docIds = createIndexWithDocs(indexName, SONGS[0], SONGS[1]);

try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) {
TestRestClient.HttpResponse mappingsResponse = client.get(indexName + "/_mapping");
mappingsResponse.assertStatusCode(SC_OK);
assertThat(mappingsResponse.getBody(), containsString("title"));

TestRestClient.HttpResponse closeResponse = client.post(indexName + "/_close");
closeResponse.assertStatusCode(SC_OK);
}

try (TestRestClient client = cluster.getRestClient(USER_NO_FIELD_TITLE_WILDCARD_INDEX_FLS)) {
TestRestClient.HttpResponse mappingsResponse = client.get(indexName + "/_mapping");
mappingsResponse.assertStatusCode(SC_OK);
assertThat(mappingsResponse.getBody(), not(containsString("title")));
}
}

}
24 changes: 18 additions & 6 deletions src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ public EvaluatedDlsFlsConfig getDlsFls(

for (SecurityRole role : roles) {
for (IndexPattern ip : role.getIpatterns()) {
final Set<String> concreteIndices = ip.concreteIndexNames(user, resolver, cs);
final Set<String> concreteIndices = ip.concreteIndexNames(user, resolver, cs, true);
String dls = ip.getDlsQuery(user);

if (dls != null && dls.length() > 0) {
Expand Down Expand Up @@ -758,24 +758,36 @@ public String getUnresolvedIndexPattern(User user) {
return UserAttributes.replaceProperties(indexPattern, user);
}

/** Finds the indices accessible to the user and resolves them to concrete names */
public Set<String> concreteIndexNames(
final User user,
final IndexNameExpressionResolver resolver,
final ClusterService cs,
final boolean includeClosed
) {
return getResolvedIndexPattern(user, resolver, cs, false, includeClosed);
}

/** Finds the indices accessible to the user and resolves them to concrete names */
public Set<String> concreteIndexNames(final User user, final IndexNameExpressionResolver resolver, final ClusterService cs) {
return getResolvedIndexPattern(user, resolver, cs, false);
return getResolvedIndexPattern(user, resolver, cs, false, false);
}

/** Finds the indices accessible to the user and attempts to resolve them to names, also includes any unresolved names */
public Set<String> attemptResolveIndexNames(final User user, final IndexNameExpressionResolver resolver, final ClusterService cs) {
return getResolvedIndexPattern(user, resolver, cs, true);
return getResolvedIndexPattern(user, resolver, cs, true, false);
}

public Set<String> getResolvedIndexPattern(
final User user,
final IndexNameExpressionResolver resolver,
final ClusterService cs,
final boolean appendUnresolved
final boolean appendUnresolved,
final boolean includeClosed
) {
final String unresolved = getUnresolvedIndexPattern(user);
final ImmutableSet.Builder<String> resolvedIndices = new ImmutableSet.Builder<>();
final IndicesOptions expansionMode = includeClosed ? IndicesOptions.lenientExpand() : IndicesOptions.lenientExpandOpen();

final WildcardMatcher matcher = WildcardMatcher.from(unresolved);
boolean includeDataStreams = true;
Expand All @@ -792,7 +804,7 @@ public Set<String> getResolvedIndexPattern(
if (aliasesAndDataStreamsForPermittedPattern.length > 0) {
final String[] resolvedAliasesAndDataStreamIndices = resolver.concreteIndexNames(
cs.state(),
IndicesOptions.lenientExpandOpen(),
expansionMode,
includeDataStreams,
aliasesAndDataStreamsForPermittedPattern
);
Expand All @@ -803,7 +815,7 @@ public Set<String> getResolvedIndexPattern(
if (!(unresolved == null || unresolved.isBlank())) {
final String[] resolvedIndicesFromPattern = resolver.concreteIndexNames(
cs.state(),
IndicesOptions.lenientExpandOpen(),
expansionMode,
includeDataStreams,
unresolved
);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public void setup(
when(log.isDebugEnabled()).thenReturn(true);
when(log.isInfoEnabled()).thenReturn(true);

doReturn(ImmutableSet.of(index)).when(ip).getResolvedIndexPattern(user, indexNameExpressionResolver, cs, true);
doReturn(ImmutableSet.of(index)).when(ip).getResolvedIndexPattern(user, indexNameExpressionResolver, cs, true, false);
}

@After
Expand Down
26 changes: 22 additions & 4 deletions src/test/java/org/opensearch/security/securityconf/impl/v7/IndexPatternTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,27 +79,27 @@ public void testCtor() {
/** Ensure that concreteIndexNames sends correct parameters are sent to getResolvedIndexPattern */
@Test
public void testConcreteIndexNamesOverload() {
doReturn(ImmutableSet.of("darn")).when(ip).getResolvedIndexPattern(user, resolver, clusterService, false);
doReturn(ImmutableSet.of("darn")).when(ip).getResolvedIndexPattern(user, resolver, clusterService, false, false);

final Set<String> results = ip.concreteIndexNames(user, resolver, clusterService);

assertThat(results, contains("darn"));

verify(ip).getResolvedIndexPattern(user, resolver, clusterService, false);
verify(ip).getResolvedIndexPattern(user, resolver, clusterService, false, false);
verify(ip).concreteIndexNames(user, resolver, clusterService);
verifyNoMoreInteractions(ip);
}

/** Ensure that attemptResolveIndexNames sends correct parameters are sent to getResolvedIndexPattern */
@Test
public void testAttemptResolveIndexNamesOverload() {
doReturn(ImmutableSet.of("yarn")).when(ip).getResolvedIndexPattern(user, resolver, clusterService, true);
doReturn(ImmutableSet.of("yarn")).when(ip).getResolvedIndexPattern(user, resolver, clusterService, true, false);

final Set<String> results = ip.attemptResolveIndexNames(user, resolver, clusterService);

assertThat(results, contains("yarn"));

verify(ip).getResolvedIndexPattern(user, resolver, clusterService, true);
verify(ip).getResolvedIndexPattern(user, resolver, clusterService, true, false);
verify(ip).attemptResolveIndexNames(user, resolver, clusterService);
verifyNoMoreInteractions(ip);
}
Expand Down Expand Up @@ -140,6 +140,24 @@ public void testExactName() {
verify(resolver).concreteIndexNames(any(), eq(IndicesOptions.lenientExpandOpen()), eq(true), eq("index-17"));
}

/** Verify concreteIndexNames on exact name matches */
@Test
public void testExactNameOnClosedIndex() {
doReturn("index-17").when(ip).getUnresolvedIndexPattern(user);
when(clusterService.state()).thenReturn(mock(ClusterState.class));
when(resolver.concreteIndexNames(any(), eq(IndicesOptions.lenientExpand()), eq(true), eq("index-17"))).thenReturn(
new String[] { "resolved-index-17" }
);

final Set<String> results = ip.concreteIndexNames(user, resolver, clusterService, true);

assertThat(results, contains("resolved-index-17"));

verify(clusterService).state();
verify(ip).getUnresolvedIndexPattern(user);
verify(resolver).concreteIndexNames(any(), eq(IndicesOptions.lenientExpand()), eq(true), eq("index-17"));
}

/** Verify concreteIndexNames on multiple matches */
@Test
public void testMultipleConcreteIndices() {
Expand Down

0 comments on commit 99457b1

Please sign in to comment.