Merge branch 'opensearch-project:main' into main

Author: kokibas

CHANGELOG.md

@@ -8,6 +8,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
 ### Enhancements
 ### Bug Fixes
 - Fix substitution templates for OpenSearch 3.x compatibility by removing `_doc` mapping ([#1301](https://github.com/opensearch-project/flow-framework/pull/1301))
+- Fixed user permission validation to correctly handle null users and backend roles ([#1292](https://github.com/opensearch-project/flow-framework/pull/1292))
 
 ### Infrastructure
 ### Documentation

build.gradle

@@ -61,7 +61,7 @@ buildscript {
 
     dependencies {
         classpath "org.opensearch.gradle:build-tools:${opensearch_version}"
-        classpath "com.diffplug.spotless:spotless-plugin-gradle:8.2.0"
+        classpath "com.diffplug.spotless:spotless-plugin-gradle:8.2.1"
     }
 }
 

src/main/java/org/opensearch/flowframework/util/ParseUtils.java

@@ -55,11 +55,13 @@
 import java.io.InputStream;
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.regex.Matcher;
@@ -366,6 +368,10 @@ public static void resolveUserAndExecute(
         }
     }
 
+    private static boolean anyNullUserOrRoles(User... users) {
+        return Arrays.stream(users).anyMatch(u -> Objects.isNull(u) || Objects.isNull(u.getBackendRoles()));
+    }
+
     /**
      * Check if requested user has backend role required to access the resource
      * @param requestedUser the user to execute the request
@@ -374,10 +380,10 @@ public static void resolveUserAndExecute(
      * @return boolean if the requested user has backend role required to access the resource
      * @throws Exception exception
      */
-    private static boolean checkUserPermissions(User requestedUser, User resourceUser, String workflowId) throws Exception {
-        if (resourceUser.getBackendRoles() == null || requestedUser.getBackendRoles() == null) {
-            return false;
-        }
+    static boolean checkUserPermissions(User requestedUser, User resourceUser, String workflowId) throws Exception {
+
+        if (anyNullUserOrRoles(requestedUser, resourceUser)) return false;
+
         // Check if requested user has backend role required to access the resource
         for (String backendRole : requestedUser.getBackendRoles()) {
             if (resourceUser.getBackendRoles().contains(backendRole)) {
@@ -533,8 +539,8 @@ public static void onGetWorkflowResponse(
                 }
                 if (shouldUseResourceAuthz(CommonValue.WORKFLOW_RESOURCE_TYPE)
                     || !filterByEnabled
-                    || checkUserPermissions(requestUser, resourceUser, workflowId)
-                    || isAdmin(requestUser)) {
+                    || isAdmin(requestUser)
+                    || checkUserPermissions(requestUser, resourceUser, workflowId)) {
                     function.run();
                 } else {
                     logger.debug("User: " + requestUser.getName() + " does not have permissions to access workflow: " + workflowId);

src/test/java/org/opensearch/flowframework/util/ParseUtilsTests.java

@@ -444,4 +444,36 @@ public void testIsAdminBackendRoleIsAllAccess() {
     public void testIsAdminNull() {
         assertFalse(isAdmin(null));
     }
+
+    public void testCheckUserPermissionsWithNullUsers() throws Exception {
+        String workflowId = "testWorkflowId";
+        User validUser = new User("user", ImmutableList.of("role"), ImmutableList.of(), ImmutableList.of());
+        User userWithNullRoles = new User("user", null, ImmutableList.of(), ImmutableList.of());
+
+        // requestedUser is null
+        assertFalse(ParseUtils.checkUserPermissions(null, validUser, workflowId));
+        // resourceUser is null
+        assertFalse(ParseUtils.checkUserPermissions(validUser, null, workflowId));
+        // resourceUser.getBackendRoles() is null
+        assertFalse(ParseUtils.checkUserPermissions(validUser, userWithNullRoles, workflowId));
+        // requestedUser.getBackendRoles() is null
+        assertFalse(ParseUtils.checkUserPermissions(userWithNullRoles, validUser, workflowId));
+    }
+
+    public void testCheckUserPermissionsWithMatchingRoles() throws Exception {
+        String workflowId = "testWorkflowId";
+        User requestedUser = new User("user1", ImmutableList.of("roleA", "roleB"), ImmutableList.of(), ImmutableList.of());
+        User resourceUser = new User("user2", ImmutableList.of("roleB", "roleC"), ImmutableList.of(), ImmutableList.of());
+
+        assertTrue(ParseUtils.checkUserPermissions(requestedUser, resourceUser, workflowId));
+    }
+
+    public void testCheckUserPermissionsWithNoMatchingRoles() throws Exception {
+        String workflowId = "testWorkflowId";
+        User requestedUser = new User("user1", ImmutableList.of("roleA"), ImmutableList.of(), ImmutableList.of());
+        User resourceUser = new User("user2", ImmutableList.of("roleB"), ImmutableList.of(), ImmutableList.of());
+
+        assertFalse(ParseUtils.checkUserPermissions(requestedUser, resourceUser, workflowId));
+    }
+
 }

src/test/java/org/opensearch/flowframework/workflow/CreateConnectorStepTests.java

@@ -71,7 +71,7 @@ public void setUp() throws Exception {
                 Map.entry(CommonValue.NAME_FIELD, "test"),
                 Map.entry(CommonValue.DESCRIPTION_FIELD, "description"),
                 Map.entry(CommonValue.VERSION_FIELD, "1"),
-                Map.entry(CommonValue.PROTOCOL_FIELD, "test"),
+                Map.entry(CommonValue.PROTOCOL_FIELD, "http"),
                 Map.entry(CommonValue.PARAMETERS_FIELD, params),
                 Map.entry(CommonValue.CREDENTIAL_FIELD, credentials),
                 Map.entry(CommonValue.ACTIONS_FIELD, actions)