Skip to content

Commit 8e9e583

Browse files
opensearch-trigger-bot[bot]peternied
opensearch-trigger-bot[bot]
and
peternied
authored
Update indices resolution to be clearer (#1999) (#2003)
(cherry picked from commit 42b936e) Co-authored-by: Peter Nied <[email protected]>
1 parent f0cfbbc commit 8e9e583

File tree

6 files changed

+491
-22
lines changed

6 files changed

+491
-22
lines changed

‎src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java‎

Lines changed: 29 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import com.google.common.collect.SetMultimap;
4646
import org.apache.logging.log4j.LogManager;
4747
import org.apache.logging.log4j.Logger;
48+
import org.apache.logging.log4j.util.Strings;
4849

4950
import org.opensearch.ExceptionsHelper;
5051
import org.opensearch.action.support.IndicesOptions;
@@ -379,8 +380,7 @@ public EvaluatedDlsFlsConfig getDlsFls(User user, boolean dfmEmptyOverwritesAll,
379380

380381
for (SecurityRole role : roles) {
381382
for (IndexPattern ip : role.getIpatterns()) {
382-
Set<String> concreteIndices;
383-
concreteIndices = ip.getResolvedIndexPattern(user, resolver, cs, false);
383+
final Set<String> concreteIndices = ip.concreteIndexNames(user, resolver, cs);
384384
String dls = ip.getDlsQuery(user);
385385

386386
if (dls != null && dls.length() > 0) {
@@ -561,7 +561,7 @@ private Set<String> getAllResolvedPermittedIndices(Resolved resolved, User user,
561561
// }
562562
if (patternMatch) {
563563
//resolved but can contain patterns for nonexistent indices
564-
final WildcardMatcher permitted = WildcardMatcher.from(p.getResolvedIndexPattern(user, resolver, cs, true)); //maybe they do not exist
564+
final WildcardMatcher permitted = WildcardMatcher.from(p.attemptResolveIndexNames(user, resolver, cs)); //maybe they do not exist
565565
final Set<String> res = new HashSet<>();
566566
if (!resolved.isLocalAll() && !resolved.getAllIndices().contains("*") && !resolved.getAllIndices().contains("_all")) {
567567
//resolved but can contain patterns for nonexistent indices
@@ -753,35 +753,42 @@ public String getUnresolvedIndexPattern(User user) {
753753
return replaceProperties(indexPattern, user);
754754
}
755755

756-
public Set<String> getResolvedIndexPattern(User user, IndexNameExpressionResolver resolver, ClusterService cs, boolean appendUnresolved) {
757-
String unresolved = getUnresolvedIndexPattern(user);
758-
WildcardMatcher matcher = WildcardMatcher.from(unresolved);
759-
String[] resolved = null;
756+
/** Finds the indices accessible to the user and resolves them to concrete names */
757+
public Set<String> concreteIndexNames(final User user, final IndexNameExpressionResolver resolver, final ClusterService cs) {
758+
return getResolvedIndexPattern(user, resolver, cs, false);
759+
}
760+
761+
/** Finds the indices accessible to the user and attempts to resolve them to names, also includes any unresolved names */
762+
public Set<String> attemptResolveIndexNames(final User user, final IndexNameExpressionResolver resolver, final ClusterService cs) {
763+
return getResolvedIndexPattern(user, resolver, cs, true);
764+
}
765+
766+
public Set<String> getResolvedIndexPattern(final User user, final IndexNameExpressionResolver resolver, final ClusterService cs, final boolean appendUnresolved) {
767+
final String unresolved = getUnresolvedIndexPattern(user);
768+
final ImmutableSet.Builder<String> resolvedIndices = new ImmutableSet.Builder<>();
769+
770+
final WildcardMatcher matcher = WildcardMatcher.from(unresolved);
760771
if (!(matcher instanceof WildcardMatcher.Exact)) {
761772
final String[] aliasesForPermittedPattern = cs.state().getMetadata().getIndicesLookup().entrySet().stream()
762773
.filter(e -> e.getValue().getType() == ALIAS)
763774
.filter(e -> matcher.test(e.getKey()))
764775
.map(e -> e.getKey())
765776
.toArray(String[]::new);
766-
767777
if (aliasesForPermittedPattern.length > 0) {
768-
resolved = resolver.concreteIndexNames(cs.state(), IndicesOptions.lenientExpandOpen(), aliasesForPermittedPattern);
778+
final String[] resolvedAliases = resolver.concreteIndexNames(cs.state(), IndicesOptions.lenientExpandOpen(), aliasesForPermittedPattern);
779+
resolvedIndices.addAll(Arrays.asList(resolvedAliases));
769780
}
770781
}
771782

772-
if (resolved == null && !unresolved.isEmpty()) {
773-
resolved = resolver.concreteIndexNames(cs.state(), IndicesOptions.lenientExpandOpen(), unresolved);
783+
if (Strings.isNotBlank(unresolved)) {
784+
final String[] resolvedIndicesFromPattern = resolver.concreteIndexNames(cs.state(), IndicesOptions.lenientExpandOpen(), unresolved);
785+
resolvedIndices.addAll(Arrays.asList(resolvedIndicesFromPattern));
774786
}
775-
if (resolved == null || resolved.length == 0) {
776-
return ImmutableSet.of(unresolved);
777-
} else {
778-
ImmutableSet.Builder<String> builder = ImmutableSet.<String>builder()
779-
.addAll(Arrays.asList(resolved));
780-
if (appendUnresolved) {
781-
builder.add(unresolved);
782-
}
783-
return builder.build();
787+
788+
if (appendUnresolved || resolvedIndices.build().isEmpty()) {
789+
resolvedIndices.add(unresolved);
784790
}
791+
return resolvedIndices.build();
785792
}
786793

787794
public String getDlsQuery(User user) {
@@ -996,12 +1003,12 @@ private static boolean impliesTypePerm(Set<IndexPattern> ipatterns, Resolved res
9961003
indexMatcherAndPermissions = ipatterns
9971004
.stream()
9981005
.filter(indexPattern -> "*".equals(indexPattern.getUnresolvedIndexPattern(user)))
999-
.map(p -> new IndexMatcherAndPermissions(p.getResolvedIndexPattern(user, resolver, cs, true), p.perms))
1006+
.map(p -> new IndexMatcherAndPermissions(p.attemptResolveIndexNames(user, resolver, cs), p.perms))
10001007
.toArray(IndexMatcherAndPermissions[]::new);
10011008
} else {
10021009
indexMatcherAndPermissions = ipatterns
10031010
.stream()
1004-
.map(p -> new IndexMatcherAndPermissions(p.getResolvedIndexPattern(user, resolver, cs, true), p.perms))
1011+
.map(p -> new IndexMatcherAndPermissions(p.attemptResolveIndexNames(user, resolver, cs), p.perms))
10051012
.toArray(IndexMatcherAndPermissions[]::new);
10061013
}
10071014
return resolvedRequestedIndices

‎src/test/java/org/opensearch/security/dlic/dlsfls/FlsIndexingTests.java‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*
8+
* Modifications Copyright OpenSearch Contributors. See
9+
* GitHub history for details.
10+
*/
11+
12+
package org.opensearch.security.dlic.dlsfls;
13+
14+
import org.apache.http.Header;
15+
import org.apache.http.HttpStatus;
16+
import org.junit.Test;
17+
18+
import org.opensearch.action.index.IndexRequest;
19+
import org.opensearch.action.support.WriteRequest.RefreshPolicy;
20+
import org.opensearch.client.Client;
21+
import org.opensearch.common.xcontent.XContentType;
22+
import org.opensearch.security.test.DynamicSecurityConfig;
23+
import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse;
24+
25+
import static org.hamcrest.MatcherAssert.assertThat;
26+
import static org.hamcrest.core.IsEqual.equalTo;
27+
import static org.hamcrest.core.IsNot.not;
28+
import static org.hamcrest.core.StringContains.containsString;
29+
30+
public class FlsIndexingTests extends AbstractDlsFlsTest {
31+
32+
protected void populateData(final Client tc) {
33+
// Create several documents in different indices with shared field names,
34+
// different roles will have different levels of FLS restrictions
35+
tc.index(new IndexRequest("yellow-pages").id("1").setRefreshPolicy(RefreshPolicy.IMMEDIATE)
36+
.source("{\"phone-all\":1001,\"phone-some\":1002,\"phone-one\":1003}", XContentType.JSON)).actionGet();
37+
tc.index(new IndexRequest("green-pages").id("2").setRefreshPolicy(RefreshPolicy.IMMEDIATE)
38+
.source("{\"phone-all\":2001,\"phone-some\":2002,\"phone-one\":2003}", XContentType.JSON)).actionGet();
39+
tc.index(new IndexRequest("blue-book").id("3").setRefreshPolicy(RefreshPolicy.IMMEDIATE)
40+
.source("{\"phone-all\":3001,\"phone-some\":3002,\"phone-one\":3003}", XContentType.JSON)).actionGet();
41+
42+
// Seperate index used to test aliasing
43+
tc.index(new IndexRequest(".hidden").id("1").setRefreshPolicy(RefreshPolicy.IMMEDIATE)
44+
.source("{}", XContentType.JSON)).actionGet();
45+
}
46+
47+
private Header asPhoneOneUser = encodeBasicHeader("user_aaa", "password");
48+
private Header asPhoneSomeUser = encodeBasicHeader("user_bbb", "password");
49+
private Header asPhoneAllUser = encodeBasicHeader("user_ccc", "password");
50+
51+
private final String searchQuery = "/*/_search?filter_path=hits.hits&pretty";
52+
53+
@Test
54+
public void testSingleIndexFlsApplied() throws Exception {
55+
setup(new DynamicSecurityConfig()
56+
.setSecurityRoles("roles_fls_indexing.yml")
57+
.setSecurityRolesMapping("roles_mapping_fls_indexing.yml"));
58+
59+
final HttpResponse phoneOneFilteredResponse = rh.executeGetRequest(searchQuery, asPhoneOneUser);
60+
assertThat(phoneOneFilteredResponse.getStatusCode(), equalTo(HttpStatus.SC_OK));
61+
assertThat(phoneOneFilteredResponse.getBody(), not(containsString("1003")));
62+
assertThat(phoneOneFilteredResponse.getBody(), containsString("1002"));
63+
assertThat(phoneOneFilteredResponse.getBody(), containsString("1001"));
64+
65+
assertThat(phoneOneFilteredResponse.getBody(), containsString("2003"));
66+
assertThat(phoneOneFilteredResponse.getBody(), containsString("2002"));
67+
assertThat(phoneOneFilteredResponse.getBody(), containsString("2001"));
68+
69+
assertThat(phoneOneFilteredResponse.getBody(), containsString("3003"));
70+
assertThat(phoneOneFilteredResponse.getBody(), containsString("3002"));
71+
assertThat(phoneOneFilteredResponse.getBody(), containsString("3001"));
72+
}
73+
74+
@Test
75+
public void testSingleIndexFlsAppliedForLimitedResults() throws Exception {
76+
setup(new DynamicSecurityConfig()
77+
.setSecurityRoles("roles_fls_indexing.yml")
78+
.setSecurityRolesMapping("roles_mapping_fls_indexing.yml"));
79+
80+
final HttpResponse phoneOneFilteredResponse = rh.executeGetRequest("/yellow-pages/_search?filter_path=hits.hits&pretty", asPhoneOneUser);
81+
assertThat(phoneOneFilteredResponse.getStatusCode(), equalTo(HttpStatus.SC_OK));
82+
assertThat(phoneOneFilteredResponse.getBody(), not(containsString("1003")));
83+
assertThat(phoneOneFilteredResponse.getBody(), containsString("1002"));
84+
assertThat(phoneOneFilteredResponse.getBody(), containsString("1001"));
85+
86+
assertThat(phoneOneFilteredResponse.getBody(), not(containsString("2003")));
87+
assertThat(phoneOneFilteredResponse.getBody(), not(containsString("2002")));
88+
assertThat(phoneOneFilteredResponse.getBody(), not(containsString("2001")));
89+
90+
assertThat(phoneOneFilteredResponse.getBody(), not(containsString("3003")));
91+
assertThat(phoneOneFilteredResponse.getBody(), not(containsString("3002")));
92+
assertThat(phoneOneFilteredResponse.getBody(), not(containsString("3001")));
93+
}
94+
95+
@Test
96+
public void testSeveralIndexFlsApplied() throws Exception {
97+
setup(new DynamicSecurityConfig()
98+
.setSecurityRoles("roles_fls_indexing.yml")
99+
.setSecurityRolesMapping("roles_mapping_fls_indexing.yml"));
100+
101+
final HttpResponse phoneSomeFilteredResponse = rh.executeGetRequest(searchQuery, asPhoneSomeUser);
102+
assertThat(phoneSomeFilteredResponse.getStatusCode(), equalTo(HttpStatus.SC_OK));
103+
assertThat(phoneSomeFilteredResponse.getBody(), containsString("1003"));
104+
assertThat(phoneSomeFilteredResponse.getBody(), not(containsString("1002")));
105+
assertThat(phoneSomeFilteredResponse.getBody(), containsString("1001"));
106+
107+
assertThat(phoneSomeFilteredResponse.getBody(), containsString("2003"));
108+
assertThat(phoneSomeFilteredResponse.getBody(), not(containsString("2002")));
109+
assertThat(phoneSomeFilteredResponse.getBody(), containsString("2001"));
110+
111+
assertThat(phoneSomeFilteredResponse.getBody(), containsString("3003"));
112+
assertThat(phoneSomeFilteredResponse.getBody(), containsString("3002"));
113+
assertThat(phoneSomeFilteredResponse.getBody(), containsString("3001"));
114+
}
115+
116+
@Test
117+
public void testAllIndexFlsApplied() throws Exception {
118+
setup(new DynamicSecurityConfig()
119+
.setSecurityRoles("roles_fls_indexing.yml")
120+
.setSecurityRolesMapping("roles_mapping_fls_indexing.yml"));
121+
122+
final HttpResponse phoneAllFilteredResponse = rh.executeGetRequest(searchQuery, asPhoneAllUser);
123+
assertThat(phoneAllFilteredResponse.getStatusCode(), equalTo(HttpStatus.SC_OK));
124+
assertThat(phoneAllFilteredResponse.getBody(), containsString("1003"));
125+
assertThat(phoneAllFilteredResponse.getBody(), containsString("1002"));
126+
assertThat(phoneAllFilteredResponse.getBody(), not(containsString("1001")));
127+
128+
assertThat(phoneAllFilteredResponse.getBody(), containsString("2003"));
129+
assertThat(phoneAllFilteredResponse.getBody(), containsString("2002"));
130+
assertThat(phoneAllFilteredResponse.getBody(), not(containsString("2001")));
131+
132+
assertThat(phoneAllFilteredResponse.getBody(), containsString("3003"));
133+
assertThat(phoneAllFilteredResponse.getBody(), containsString("3002"));
134+
assertThat(phoneAllFilteredResponse.getBody(), not(containsString("3001")));
135+
}
136+
137+
@Test
138+
public void testAllIndexFlsAppliedWithAlias() throws Exception {
139+
setup(new DynamicSecurityConfig()
140+
.setSecurityRoles("roles_fls_indexing.yml")
141+
.setSecurityRolesMapping("roles_mapping_fls_indexing.yml"));
142+
143+
final HttpResponse createAlias = rh.executePostRequest("_aliases", "{\"actions\":[{\"add\":{\"index\":\".hidden\",\"alias\":\"ducky\"}}]}", asPhoneAllUser);
144+
assertThat(createAlias.getStatusCode(), equalTo(HttpStatus.SC_OK));
145+
146+
final HttpResponse phoneAllFilteredResponse = rh.executeGetRequest(searchQuery, asPhoneAllUser);
147+
assertThat(phoneAllFilteredResponse.getStatusCode(), equalTo(HttpStatus.SC_OK));
148+
assertThat(phoneAllFilteredResponse.getBody(), containsString("1003"));
149+
assertThat(phoneAllFilteredResponse.getBody(), containsString("1002"));
150+
assertThat(phoneAllFilteredResponse.getBody(), not(containsString("1001")));
151+
152+
assertThat(phoneAllFilteredResponse.getBody(), containsString("2003"));
153+
assertThat(phoneAllFilteredResponse.getBody(), containsString("2002"));
154+
assertThat(phoneAllFilteredResponse.getBody(), not(containsString("2001")));
155+
156+
assertThat(phoneAllFilteredResponse.getBody(), containsString("3003"));
157+
assertThat(phoneAllFilteredResponse.getBody(), containsString("3002"));
158+
assertThat(phoneAllFilteredResponse.getBody(), not(containsString("3001")));
159+
}
160+
}

0 commit comments

Comments
 (0)