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.

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

Conversation

AfraHussaindeen
Copy link

Purpose

Screen.Recording.2025-02-05.at.12.44.47.mov

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 44.41%. Comparing base (67df964) to head (247143a).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
.../identity/scim2/common/impl/SCIMRoleManagerV2.java 50.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #606      +/-   ##
============================================
- Coverage     47.33%   44.41%   -2.93%     
- Complexity     1130     1160      +30     
============================================
  Files            41       41              
  Lines          9141    10139     +998     
  Branches       2432     2812     +380     
============================================
+ Hits           4327     4503     +176     
- Misses         3886     4677     +791     
- Partials        928      959      +31     
Flag Coverage Δ
unit 31.27% <50.00%> (-1.65%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

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

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).

* @param attributeValue Value of the attribute
* @return Formatted attribute value for search
*/
private String formatSearchAttributeValue(String filterOperation, String attributeValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we test this with multiple filters.

name co and

How this is processed through the filtering nodes.
As there is no value after and operation, if we can process and part as a value.

Copy link
Author

@AfraHussaindeen AfraHussaindeen Feb 5, 2025

Choose a reason for hiding this comment

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

When using a filter like:

name co and,

and is treated as part of the search value (not a logical operator). For more information refer #606 (comment)

@AfraHussaindeen
Copy link
Author

AfraHussaindeen commented Feb 6, 2025

Upon calling the GET /scim2/v2/Roles API with filtering option, the listWithGETRole() method is executed, leading to the creation of a Node associated with the filter (Node creation). The FilterTreeManager class is then used to build the filter tree, where the logic (Filter Processing) processes the filter string and generates an intermediate filter expressions list (tempTokenList).

For example, if the filter string is audienceId ne test and displayName co or, the tempTokenList will be ["audienceId ne test", "and", "displayName co", "or"]. Subsequently, another logic step (Final Token List Generation) converts this into the final filter expressions list(tokenList) as ["audienceId ne test", "and", "displayName co or"], ensuring proper expression generation for processing.

Next, the listRolesWithGET() method is invoked, which internally invokebuildSearchFilter(), which constructs and returns the search filter string ("audienceId ne test and displayName co or") to the role service layer (SCIMRoleManagerV2.java#L498).

Within the role service layer, getExpressionNodes() is called, utilizing the FilterTreeBuilder class to construct the filter tree.

The root cause of the internal server error occurs at this stage. Within FilterTreeBuilder.java, the filter expression list(token list) remains unchanged: ["audienceId ne test", "and", "displayName co", "or"] as no additional processing is defined to handleand andor correctly when it is expected to be value string.
Hence, when the system attempts to build the filter tree, it fails leading to an exception.

To resolve this issue, two possible solutions are available:

  • Modify the SCIM layer : Before passing the filter string to the service layer, wrap value strings (ex:or) in single quotes (ex:'or') so that it is correctly treated as an attribute value rather than a operator.
  • Modify FilterTreeBuilder Logic: Update the FilterTreeBuilder class to handle and and or in a manner similar to FilterTreeManager.

The first approach is preferable as it ensures correct filtering logic before reaching the service layer, preventing unnecessary complexities in FilterTreeBuilder.

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/13171396437

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/13171396437
Status: failure

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/13175259984

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/13175259984
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

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

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/13175259984

@AfraHussaindeen
Copy link
Author

Closing this pr as it was decided to proceed with the Option 2 : Modify FilterTreeBuilder Logic. For the fix refer wso2/carbon-identity-framework#6440.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filtering roles with special characters give 500 error response
4 participants