diff --git a/CHANGELOG.md b/CHANGELOG.md index 90c502440..25ba2f45a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) ### Features ### Enhancements ### Bug Fixes +- Fixed user permission validation to correctly handle null users and backend roles. ### Infrastructure ### Documentation ### Maintenance diff --git a/src/main/java/org/opensearch/flowframework/util/ParseUtils.java b/src/main/java/org/opensearch/flowframework/util/ParseUtils.java index 0e417ef94..546eafce0 100644 --- a/src/main/java/org/opensearch/flowframework/util/ParseUtils.java +++ b/src/main/java/org/opensearch/flowframework/util/ParseUtils.java @@ -366,6 +366,16 @@ public static void resolveUserAndExecute( } } + // helper method to check if varargs User... is null + private static boolean hasUsersAndRoles(User requestedUser, User resourceUser) { + + return requestedUser != null + && resourceUser != null + && resourceUser.getBackendRoles() != null + && requestedUser.getBackendRoles() != null; + + } + /** * Check if requested user has backend role required to access the resource * @param requestedUser the user to execute the request @@ -374,10 +384,11 @@ 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; - } + + if (!hasUsersAndRoles(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)) { @@ -395,6 +406,13 @@ private static boolean checkUserPermissions(User requestedUser, User resourceUse return false; } + // method to expose checkUserPermissions for testing + static boolean exposeCheckUserPermissions(User requestedUser, User resourceUser, String workflowId) throws Exception { + + return checkUserPermissions(requestedUser, resourceUser, workflowId); + + } + /** * Check if filter by backend roles is enabled and validate the requested user * @param requestedUser the user to execute the request @@ -533,8 +551,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); diff --git a/src/test/java/org/opensearch/flowframework/util/ParseUtilsTests.java b/src/test/java/org/opensearch/flowframework/util/ParseUtilsTests.java index 6e63dfb97..5550b6a50 100644 --- a/src/test/java/org/opensearch/flowframework/util/ParseUtilsTests.java +++ b/src/test/java/org/opensearch/flowframework/util/ParseUtilsTests.java @@ -444,4 +444,17 @@ public void testIsAdminBackendRoleIsAllAccess() { public void testIsAdminNull() { assertFalse(isAdmin(null)); } + + public void testCheckUserPermissionsWithNullUsers() throws Exception { + + User mockrequestedUser = null; + User mockresourceUser = new User(); + String mockWorkFlowId = "mockWorkFlowId"; + + boolean res = ParseUtils.exposeCheckUserPermissions(mockrequestedUser, mockresourceUser, mockWorkFlowId); + + assertFalse(res); + + } + }