Merge branch 'main' into fix-jwt-url-param

.github/workflows/ci.yml

@@ -54,7 +54,7 @@ jobs:
       uses: actions/checkout@v4
 
     - name: Build and Test
-      uses: gradle/gradle-build-action@v2
+      uses: gradle/gradle-build-action@v3
       with:
         cache-disabled: true
         arguments: |
@@ -112,7 +112,7 @@ jobs:
       uses: actions/checkout@v4
 
     - name: Build and Test
-      uses: gradle/gradle-build-action@v2
+      uses: gradle/gradle-build-action@v3
       with:
         cache-disabled: true
         arguments: |
@@ -147,7 +147,7 @@ jobs:
       uses: actions/checkout@v4
 
     - name: Build and Test
-      uses: gradle/gradle-build-action@v2
+      uses: gradle/gradle-build-action@v3
       with:
         cache-disabled: true
         arguments: |
@@ -165,7 +165,7 @@ jobs:
         uses: actions/checkout@v4
 
       - name: Build BWC tests
-        uses: gradle/gradle-build-action@v2
+        uses: gradle/gradle-build-action@v3
         with:
           cache-disabled: true
           arguments: |

.github/workflows/code-hygiene.yml

@@ -24,7 +24,7 @@ jobs:
           distribution: temurin # Temurin is a distribution of adoptium
           java-version: 17
 
-      - uses: gradle/gradle-build-action@v2
+      - uses: gradle/gradle-build-action@v3
         with:
           cache-disabled: true
           arguments: spotlessCheck
@@ -40,7 +40,7 @@ jobs:
           distribution: temurin # Temurin is a distribution of adoptium
           java-version: 11
 
-      - uses: gradle/gradle-build-action@v2
+      - uses: gradle/gradle-build-action@v3
         with:
           cache-disabled: true
           arguments: checkstyleMain checkstyleTest checkstyleIntegrationTest
@@ -56,7 +56,7 @@ jobs:
           distribution: temurin # Temurin is a distribution of adoptium
           java-version: 11
 
-      - uses: gradle/gradle-build-action@v2
+      - uses: gradle/gradle-build-action@v3
         with:
           cache-disabled: true
           arguments: spotbugsMain

.github/workflows/plugin_install.yml

@@ -29,7 +29,7 @@ jobs:
         uses: actions/checkout@v4
 
       - name: Assemble target plugin
-        uses: gradle/gradle-build-action@v2
+        uses: gradle/gradle-build-action@v3
         with:
           cache-disabled: true
           arguments: assemble
@@ -63,7 +63,7 @@ jobs:
           admin-password: ${{ steps.random-password.outputs.generated_name }}
 
       - name: Run sanity tests
-        uses: gradle/gradle-build-action@v2
+        uses: gradle/gradle-build-action@v3
         with:
           cache-disabled: true
           arguments: integTestRemote -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9200 -Dtests.clustername="opensearch" -Dhttps=true -Duser=admin -Dpassword=${{ steps.random-password.outputs.generated_name }} -i

build.gradle

@@ -29,7 +29,7 @@ buildscript {
         apache_cxf_version = '4.0.3'
         open_saml_version = '4.3.0'
         one_login_java_saml = '2.9.0'
-        jjwt_version = '0.12.3'
+        jjwt_version = '0.12.4'
         guava_version = '32.1.3-jre'
         jaxb_version = '2.3.9'
         spring_version = '5.3.31'
@@ -62,7 +62,7 @@ plugins {
     id 'idea'
     id 'jacoco'
     id 'maven-publish'
-    id 'com.diffplug.spotless' version '6.24.0'
+    id 'com.diffplug.spotless' version '6.25.0'
     id 'checkstyle'
     id 'com.netflix.nebula.ospackage' version "11.6.0"
     id "org.gradle.test-retry" version "1.5.8"
@@ -643,7 +643,7 @@ dependencies {
     runtimeOnly 'com.google.j2objc:j2objc-annotations:2.8'
     compileOnly 'com.google.code.findbugs:jsr305:3.0.2'
     runtimeOnly 'org.lz4:lz4-java:1.8.0'
-    runtimeOnly 'io.dropwizard.metrics:metrics-core:4.2.24'
+    runtimeOnly 'io.dropwizard.metrics:metrics-core:4.2.25'
     runtimeOnly 'org.slf4j:slf4j-api:1.7.36'
     runtimeOnly "org.apache.logging.log4j:log4j-slf4j-impl:${versions.log4j}"
     runtimeOnly 'org.xerial.snappy:snappy-java:1.1.10.5'

src/integrationTest/java/org/opensearch/security/DlsIntegrationTests.java

@@ -10,12 +10,15 @@
 package org.opensearch.security;
 
 import java.io.IOException;
+import java.io.Serializable;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.function.BiFunction;
+import java.util.stream.Collectors;
 
 import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
+import org.apache.commons.lang3.tuple.Pair;
 import org.junit.BeforeClass;
 import org.junit.ClassRule;
 import org.junit.Test;
@@ -36,6 +39,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
 import static org.opensearch.action.admin.indices.alias.IndicesAliasesRequest.AliasActions.Type.ADD;
 import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE;
 import static org.opensearch.client.RequestOptions.DEFAULT;
@@ -57,6 +61,7 @@
 import static org.opensearch.test.framework.matcher.SearchResponseMatchers.isSuccessfulSearchResponse;
 import static org.opensearch.test.framework.matcher.SearchResponseMatchers.numberOfTotalHitsIsEqualTo;
 import static org.opensearch.test.framework.matcher.SearchResponseMatchers.searchHitContainsFieldWithValue;
+import static org.opensearch.test.framework.matcher.SearchResponseMatchers.searchHitsContainDocumentsInAnyOrder;
 
 @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
 @ThreadLeakScope(ThreadLeakScope.Scope.NONE)
@@ -82,6 +87,7 @@ public class DlsIntegrationTests {
     static final String FIRST_INDEX_ALIAS_FILTERED_BY_TWINS_ARTIST = FIRST_INDEX_NAME.concat("-filtered-by-twins-artist");
     static final String FIRST_INDEX_ALIAS_FILTERED_BY_FIRST_ARTIST = FIRST_INDEX_NAME.concat("-filtered-by-first-artist");
     static final String ALL_INDICES_ALIAS = "_all";
+    static final String UNION_TEST_INDEX_NAME = "my_index1";
 
     static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS);
 
@@ -158,6 +164,62 @@ public class DlsIntegrationTests {
                 .on("*")
         );
 
+    /**
+     * Test role 1 for DLS filtering with two (non)overlapping roles. This role imposes a filter where the user can only access documents where the sensitive field is false. This role is applied at a higher level for all index patterns.
+     */
+    static final TestSecurityConfig.Role ROLE_NON_SENSITIVE_ONLY = new TestSecurityConfig.Role("test_role_1").clusterPermissions(
+        "cluster_composite_ops_ro"
+    ).indexPermissions("read").dls("{\"match\":{\"sensitive\":false}}").on("*");
+
+    /**
+     * Test role 2 for DLS filtering with two overlapping roles. This role does not impose any filter, and combined with TEST_ROLE_ONE should yield a union that does not impose any filter. This role is applied at a lower level for index patterns my_index*.
+     */
+    static final TestSecurityConfig.Role ROLE_ALLOW_ALL = new TestSecurityConfig.Role("test_role_2").clusterPermissions(
+        "cluster_composite_ops_ro"
+    ).indexPermissions("read").dls("{\"match_all\": {}}").on("my_index*");
+
+    /**
+     * Test role 3 for DLS filtering with two nonoverlapping roles. This role imposes a filter where the user can only access documents where the genre field is History, and combined with TEST_ROLE_ONE should yield a union that allows the user to access every document except the one with genre Science and sensitive true. This role is applied at a lower level for index patterns my_index*.
+     */
+    static final TestSecurityConfig.Role ROLE_MATCH_HISTORY_GENRE_ONLY = new TestSecurityConfig.Role("test_role_3").clusterPermissions(
+        "cluster_composite_ops_ro"
+    ).indexPermissions("read").dls("{\"match\":{\"genre\":\"History\"}}").on("my_index*");
+
+    /**
+     * User with DLS permission to only be able to access documents with false sensitive property.
+     */
+    static final TestSecurityConfig.User USER_NON_SENSITIVE_ONLY = new TestSecurityConfig.User("test_role_1_user").roles(
+        ROLE_NON_SENSITIVE_ONLY
+    );
+
+    /**
+     * User with DLS permission to access all documents.
+     */
+    static final TestSecurityConfig.User USER_ALLOW_ALL = new TestSecurityConfig.User("test_role_2_user").roles(ROLE_ALLOW_ALL);
+
+    /**
+     * User with DLS permission to access documents with genre property matching History.
+     */
+    static final TestSecurityConfig.User USER_MATCH_HISTORY_GENRE_ONLY = new TestSecurityConfig.User("test_role_3_user").roles(
+        ROLE_MATCH_HISTORY_GENRE_ONLY
+    );
+
+    /**
+     * User with overlapping DLS permissions to access documents with false sensitive property and access all documents- should yield accessing all documents.
+     */
+    static final TestSecurityConfig.User USER_UNION_OF_OVERLAPPING_ROLES_NON_SENSITIVE_ONLY_AND_ALLOW_ALL = new TestSecurityConfig.User(
+        "test_union_of_overlapping_roles_user"
+    ).roles(ROLE_NON_SENSITIVE_ONLY, ROLE_ALLOW_ALL);
+
+    /**
+     * User with non-overlapping DLS permissions to access documents with false sensitive property and genre property matching History.
+     */
+    static final TestSecurityConfig.User USER_UNION_OF_NONOVERLAPPING_ROLES_NON_SENSITIVE_ONLY_AND_HISTORY_GENRE_ONLY =
+        new TestSecurityConfig.User("test_union_of_non_overlapping_roles_user").roles(
+            ROLE_NON_SENSITIVE_ONLY,
+            ROLE_MATCH_HISTORY_GENRE_ONLY
+        );
+
     @ClassRule
     public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS)
         .anonymousAuth(false)
@@ -172,7 +234,12 @@ public class DlsIntegrationTests {
             READ_WHERE_FIELD_ARTIST_MATCHES_ARTIST_STRING,
             READ_WHERE_STARS_LESS_THAN_THREE,
             READ_WHERE_FIELD_ARTIST_MATCHES_ARTIST_TWINS_OR_FIELD_STARS_GREATER_THAN_FIVE,
-            READ_WHERE_FIELD_ARTIST_MATCHES_ARTIST_TWINS_OR_MATCHES_ARTIST_FIRST
+            READ_WHERE_FIELD_ARTIST_MATCHES_ARTIST_TWINS_OR_MATCHES_ARTIST_FIRST,
+            USER_NON_SENSITIVE_ONLY,
+            USER_ALLOW_ALL,
+            USER_MATCH_HISTORY_GENRE_ONLY,
+            USER_UNION_OF_OVERLAPPING_ROLES_NON_SENSITIVE_ONLY_AND_ALLOW_ALL,
+            USER_UNION_OF_NONOVERLAPPING_ROLES_NON_SENSITIVE_ONLY_AND_HISTORY_GENRE_ONLY
         )
         .build();
 
@@ -218,6 +285,21 @@ public class DlsIntegrationTests {
         }
     };
 
+    static final TreeMap<String, Map<String, Serializable>> UNION_ROLE_TEST_DATA = new TreeMap<>() {
+        {
+            put("1", Map.of("genre", "History", "date", "01-01-2020", "sensitive", true));
+            put("2", Map.of("genre", "History", "date", "01-01-2020", "sensitive", true));
+            put("3", Map.of("genre", "History", "date", "01-01-2020", "sensitive", true));
+            put("4", Map.of("genre", "History", "date", "01-01-2020", "sensitive", true));
+            put("5", Map.of("genre", "History", "date", "01-01-2020", "sensitive", true));
+            put("6", Map.of("genre", "Math", "date", "01-01-2020", "sensitive", false));
+            put("7", Map.of("genre", "Math", "date", "01-01-2020", "sensitive", false));
+            put("8", Map.of("genre", "Math", "date", "01-01-2020", "sensitive", false));
+            put("9", Map.of("genre", "Math", "date", "01-01-2020", "sensitive", false));
+            put("10", Map.of("genre", "Science", "date", "01-01-2020", "sensitive", true));
+        }
+    };
+
     @BeforeClass
     public static void createTestData() {
         try (Client client = cluster.getInternalNodeClient()) {
@@ -275,6 +357,10 @@ public static void createTestData() {
                     )
                 )
                 .actionGet();
+
+            UNION_ROLE_TEST_DATA.forEach((index, document) -> {
+                client.prepareIndex(UNION_TEST_INDEX_NAME).setId(index).setRefreshPolicy(IMMEDIATE).setSource(document).get();
+            });
         }
     }
 
@@ -517,4 +603,121 @@ public void testAggregateAndComputeStarRatings() throws IOException {
             assertThat(((ParsedAvg) actualAggregation).getValue(), is(1.5));
         }
     }
+
+    @Test
+    public void testOverlappingRoleUnionSearchFiltering() throws Exception {
+        try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(USER_NON_SENSITIVE_ONLY)) {
+            SearchRequest searchRequest = new SearchRequest(UNION_TEST_INDEX_NAME);
+            SearchResponse searchResponse = restHighLevelClient.search(searchRequest, DEFAULT);
+
+            assertSearchResponseHitsEqualTo(searchResponse, 4);
+
+            assertThat(
+                searchResponse,
+                searchHitsContainDocumentsInAnyOrder(
+                    UNION_ROLE_TEST_DATA.entrySet()
+                        .stream()
+                        .filter(e -> e.getValue().get("sensitive").equals(false))
+                        .map(e -> Pair.of(UNION_TEST_INDEX_NAME, e.getKey()))
+                        .collect(Collectors.toList())
+                )
+            );
+        }
+
+        try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(USER_ALLOW_ALL)) {
+            SearchRequest searchRequest = new SearchRequest(UNION_TEST_INDEX_NAME);
+            SearchResponse searchResponse = restHighLevelClient.search(searchRequest, DEFAULT);
+
+            assertSearchResponseHitsEqualTo(searchResponse, 10);
+        }
+
+        try (
+            RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(
+                USER_UNION_OF_OVERLAPPING_ROLES_NON_SENSITIVE_ONLY_AND_ALLOW_ALL
+            )
+        ) {
+            SearchRequest searchRequest = new SearchRequest(UNION_TEST_INDEX_NAME);
+            SearchResponse searchResponse = restHighLevelClient.search(searchRequest, DEFAULT);
+
+            assertSearchResponseHitsEqualTo(searchResponse, 10);
+
+            // shows that roles are additive and the overlapping role with less filtering is used
+            assertThat(
+                searchResponse,
+                searchHitsContainDocumentsInAnyOrder(
+                    UNION_ROLE_TEST_DATA.keySet().stream().map(id -> Pair.of(UNION_TEST_INDEX_NAME, id)).collect(Collectors.toList())
+                )
+            );
+        }
+    }
+
+    @Test
+    @SuppressWarnings("unchecked")
+    public void testNonOverlappingRoleUnionSearchFiltering() throws Exception {
+        try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(USER_NON_SENSITIVE_ONLY)) {
+            SearchRequest searchRequest = new SearchRequest(UNION_TEST_INDEX_NAME);
+            SearchResponse searchResponse = restHighLevelClient.search(searchRequest, DEFAULT);
+
+            assertSearchResponseHitsEqualTo(searchResponse, 4);
+
+            assertThat(
+                searchResponse,
+                searchHitsContainDocumentsInAnyOrder(
+                    UNION_ROLE_TEST_DATA.entrySet()
+                        .stream()
+                        .filter(e -> e.getValue().get("sensitive").equals(false))
+                        .map(e -> Pair.of(UNION_TEST_INDEX_NAME, e.getKey()))
+                        .collect(Collectors.toList())
+                )
+            );
+        }
+
+        try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(USER_MATCH_HISTORY_GENRE_ONLY)) {
+            SearchRequest searchRequest = new SearchRequest(UNION_TEST_INDEX_NAME);
+            SearchResponse searchResponse = restHighLevelClient.search(searchRequest, DEFAULT);
+
+            assertSearchResponseHitsEqualTo(searchResponse, 5);
+
+            assertThat(
+                searchResponse,
+                searchHitsContainDocumentsInAnyOrder(
+                    UNION_ROLE_TEST_DATA.entrySet()
+                        .stream()
+                        .filter(e -> e.getValue().get("genre").equals("History"))
+                        .map(e -> Pair.of(UNION_TEST_INDEX_NAME, e.getKey()))
+                        .collect(Collectors.toList())
+                )
+            );
+        }
+
+        try (
+            RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(
+                USER_UNION_OF_NONOVERLAPPING_ROLES_NON_SENSITIVE_ONLY_AND_HISTORY_GENRE_ONLY
+            )
+        ) {
+            SearchRequest searchRequest = new SearchRequest(UNION_TEST_INDEX_NAME);
+            SearchResponse searchResponse = restHighLevelClient.search(searchRequest, DEFAULT);
+
+            assertSearchResponseHitsEqualTo(searchResponse, 9);
+
+            assertThat(
+                searchResponse,
+                searchHitsContainDocumentsInAnyOrder(
+                    UNION_ROLE_TEST_DATA.keySet()
+                        .stream()
+                        .filter(id -> !id.equals("10"))
+                        .map(id -> Pair.of(UNION_TEST_INDEX_NAME, id))
+                        .collect(Collectors.toList())
+                )
+            );
+
+            // shows that the roles are additive, but excludes one document since the DLS filters for both roles do not account for this
+            assertThat(searchResponse, not(searchHitsContainDocumentsInAnyOrder(Pair.of(UNION_TEST_INDEX_NAME, "10"))));
+        }
+    }
+
+    private void assertSearchResponseHitsEqualTo(SearchResponse searchResponse, int hits) throws Exception {
+        assertThat(searchResponse, isSuccessfulSearchResponse());
+        assertThat(searchResponse, numberOfTotalHitsIsEqualTo(hits));
+    }
 }

src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java

@@ -46,16 +46,16 @@ public class ActionGroupsApiAction extends AbstractApiAction {
             // legacy mapping for backwards compatibility
             // TODO: remove in next version
             new Route(Method.GET, "/actiongroup/{name}"),
-            new Route(Method.GET, "/actiongroup/"),
+            new Route(Method.GET, "/actiongroup"),
             new Route(Method.DELETE, "/actiongroup/{name}"),
             new Route(Method.PUT, "/actiongroup/{name}"),
 
             // corrected mapping, introduced in OpenSearch Security
             new Route(Method.GET, "/actiongroups/{name}"),
-            new Route(Method.GET, "/actiongroups/"),
+            new Route(Method.GET, "/actiongroups"),
             new Route(Method.DELETE, "/actiongroups/{name}"),
             new Route(Method.PUT, "/actiongroups/{name}"),
-            new Route(Method.PATCH, "/actiongroups/"),
+            new Route(Method.PATCH, "/actiongroups"),
             new Route(Method.PATCH, "/actiongroups/{name}")
 
         )