Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the format of our change log entries (see the release notes for where they eventually go) and add a blank line after this so the markdown properly renders.

### Infrastructure
### Documentation
### Maintenance
Expand Down
28 changes: 23 additions & 5 deletions src/main/java/org/opensearch/flowframework/util/ParseUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

}
Comment on lines +370 to +377
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static boolean hasUsersAndRoles(User requestedUser, User resourceUser) {
return requestedUser != null
&& resourceUser != null
&& resourceUser.getBackendRoles() != null
&& requestedUser.getBackendRoles() != null;
}
private static boolean hasUsersAndRoles(User... users) {
boolean nonNull = true;
for (User user: users) {
if (user == null || user.getBakendRoles() == null) {
nonNull = false;
break;
}
}
return nonNull;
}

Or you can possibly use a stream.


/**
* Check if requested user has backend role required to access the resource
* @param requestedUser the user to execute the request
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but given the negation we may want to make the helper method nullUsersOrRoles and reverse the true/false in it.


// Check if requested user has backend role required to access the resource
for (String backendRole : requestedUser.getBackendRoles()) {
if (resourceUser.getBackendRoles().contains(backendRole)) {
Expand All @@ -395,6 +406,13 @@ private static boolean checkUserPermissions(User requestedUser, User resourceUse
return false;
}

// method to expose checkUserPermissions for testing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to just make the existing method package private with this comment than have a wrapper method to do the same thing.

It actually wouldn't hurt to just make the method public.

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
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

}
Comment on lines +448 to +458
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good test case but you need to test all the conditional cases (null/non-null on both requested and resource user).


}
Loading