Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix server error while filtering with special characters #606

Closed
wants to merge 3 commits into from
Closed

Fix server error while filtering with special characters #606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3db19f8
Fix server error while filtering with special characters
AfraHussaindeen Feb 5, 2025
cb68d51
Add unit tests for scim role filtering
AfraHussaindeen Feb 5, 2025
247143a
Update formatted attribute logic
AfraHussaindeen Feb 6, 2025
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
27 changes: 24 additions & 3 deletions ...m2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,15 +608,17 @@ private String getSearchFilter(String attributeName, String filterOperation, Str
throws BadRequestException {

String searchFilter;
String formattedAttributeValue = formatSearchAttributeValue(attributeValue);

switch (attributeName) {
case SCIMConstants.RoleSchemaConstants.DISPLAY_NAME_URI:
searchFilter = ROLE_NAME_FILTER_ATTRIBUTE + " " + filterOperation + " " + attributeValue;
searchFilter = ROLE_NAME_FILTER_ATTRIBUTE + " " + filterOperation + " " + formattedAttributeValue;
Copy link
Contributor

@PasinduYeshan PasinduYeshan Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can you point out where these values are handled later in the flow?
Previously, the attribute value didn't include any operator-specific logic, but now with formattedAttribute, it does. I’d like to understand how this affects subsequent processing. Are there any changes in how these formatted values are utilized in query construction or filtering logic?

Copy link
Author

@AfraHussaindeen AfraHussaindeen Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Refer #606 (comment).

For an inference nf how the list of expression nodes derived from the filter string is utilized to construct the SQL query, refer to RoleDAOImpl.java (Line 396).

break;
case SCIMConstants.RoleSchemaConstants.AUDIENCE_VALUE_URI:
searchFilter = ROLE_AUDIENCE_ID_FILTER_ATTRIBUTE + " " + filterOperation + " " + attributeValue;
searchFilter = ROLE_AUDIENCE_ID_FILTER_ATTRIBUTE + " " + filterOperation + " " + formattedAttributeValue;
break;
case SCIMConstants.RoleSchemaConstants.AUDIENCE_TYPE_URI:
searchFilter = ROLE_AUDIENCE_TYPE_FILTER_ATTRIBUTE + " " + filterOperation + " " + attributeValue;
searchFilter = ROLE_AUDIENCE_TYPE_FILTER_ATTRIBUTE + " " + filterOperation + " " + formattedAttributeValue;
break;
default:
String errorMessage = "Filtering based on attribute: " + attributeName + " is not supported.";
Expand All @@ -625,6 +627,25 @@ private String getSearchFilter(String attributeName, String filterOperation, Str
return searchFilter;
}

/**
* Format the attribute value for search.
*
* @param attributeValue Value of the attribute
* @return Formatted attribute value for search
*/
private String formatSearchAttributeValue(String attributeValue) {

// Handle empty or null values
if (StringUtils.isBlank(attributeValue)) {
return "''";
}

// Escape special characters in the attribute value
String escapedValue = attributeValue.replace("'", "''");

return "'" + escapedValue + "'";
}

private List<RoleV2> getScimRolesList(List<Role> roles, List<String> requiredAttributes)
throws BadRequestException, CharonException {

Expand Down
110 changes: 109 additions & 1 deletion ...ommon/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,12 @@
import org.wso2.charon3.core.protocol.ResponseCodeConstants;
import org.wso2.charon3.core.schema.SCIMConstants;
import org.wso2.charon3.core.utils.codeutils.PatchOperation;
import org.wso2.charon3.core.utils.codeutils.Node;
import org.wso2.charon3.core.utils.codeutils.FilterTreeManager;
import org.wso2.charon3.core.schema.SCIMResourceTypeSchema;
import org.wso2.charon3.core.schema.SCIMResourceSchemaManager;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -95,10 +100,13 @@ public class SCIMRoleManagerV2Test {

private MockedStatic<IdentityUtil> identityUtil;

private static SCIMResourceTypeSchema schema;

@BeforeClass
public void setUpClass() {
public void setUpClass() throws Exception {

initMocks(this);
schema = SCIMResourceSchemaManager.getInstance().getRoleResourceV2Schema();
}

@BeforeMethod
Expand All @@ -110,6 +118,7 @@ public void setUpMethod() {

@AfterMethod
public void tearDown() {

identityUtil.close();
}

Expand Down Expand Up @@ -415,4 +424,103 @@ public void testListRolesWithGETWithRoleProperties(boolean isPropertiesRequired)
}
}
}

@DataProvider(name = "roleFilteringData")
public Object[][] getRoleFilteringData() {

return new Object[][]{
{
"displayName eq admin",
"name eq 'admin'",
new Role[]{
createMockRole(ROLE_ID, "admin"),
createMockRole(ROLE_ID_2, "administrator")
}
},
{
"displayName sw sys",
"name sw 'sys'",
new Role[]{
createMockRole(ROLE_ID, "sysadmin"),
createMockRole(ROLE_ID_2, "system_admin")
}
},
{
"displayName ew admin",
"name ew 'admin'",
new Role[]{
createMockRole(ROLE_ID, "sysadmin"),
createMockRole(ROLE_ID_2, "domain_admin")
}
},
{
"displayName co min",
"name co 'min'",
new Role[]{
createMockRole(ROLE_ID, "administrator"),
createMockRole(ROLE_ID_2, "adminuser")
}
},
{
"displayName ew or",
"name ew 'or'",
new Role[]{
createMockRole(ROLE_ID, "administrator"),
}
},
{
"displayName co and",
"name co 'and'",
new Role[]{
createMockRole(ROLE_ID, "brand_admin"),
}
}
};
}

@Test(dataProvider = "roleFilteringData")
public void testListRolesWithFiltering(String filterExpression, String expectedSQLFilter, Role[] mockRoles)
throws Exception {

try (MockedStatic<SCIMCommonUtils> mockedSCIMCommonUtils = mockStatic(SCIMCommonUtils.class)) {

for (Role role : mockRoles) {
mockedSCIMCommonUtils.when(() -> SCIMCommonUtils.getSCIMRoleV2URL(role.getId()))
.thenReturn(SCIM2_ROLES_V2_LOCATION_URI_BASE + role.getId());
}

when(roleManagementService.getRoles(eq(expectedSQLFilter), any(), any(), any(), any(),
eq(SAMPLE_TENANT_DOMAIN), any())).thenReturn(Arrays.asList(mockRoles));

Node filterNode = buildNode(filterExpression, schema);
RolesV2GetResponse response = scimRoleManagerV2.listRolesWithGET(
filterNode, 1, 10, null, null, null);

// Verify results
List<RoleV2> returnedRoles = response.getRoles();
assertEquals(returnedRoles.size(), mockRoles.length,
"Returned roles size should match for filter: " + filterExpression);
for (int i = 0; i < mockRoles.length; i++) {
assertEquals(returnedRoles.get(i).getDisplayName(), mockRoles[i].getName(),
"Role name should match for filter: " + filterExpression);
}
}
}

private Role createMockRole(String id, String name) {

Role role = new Role();
role.setId(id);
role.setName(name);
return role;
}

private Node buildNode(String filter, SCIMResourceTypeSchema schema) throws BadRequestException, IOException {

if (filter != null) {
FilterTreeManager filterTreeManager = new FilterTreeManager(filter, schema);
return filterTreeManager.buildTree();
}
return null;
}
}