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

Wildcard is not used now by default. User has to set it explicitly if needed #2277

Merged
merged 2 commits into from
May 24, 2023

Conversation

ssuvorov-fls
Copy link
Contributor

fixes #1790

@ssuvorov-fls ssuvorov-fls requested a review from chrisknoll May 24, 2023 11:56
@ssuvorov-fls ssuvorov-fls changed the title Wildcard is not used now by default. User have to set it explicitly if needed Wildcard is not used now by default. User has to set it explicitly if needed May 24, 2023
Comment on lines 42 to 73
private String encodeSearchString(String searchString) {
// encode search string without encoding wildcards
if (searchString == null) {
return null;
}
char wildcard = '*';
List<Integer> wildcardIndexes = IntStream.range(0, searchString.length())
.filter(i -> searchString.charAt(i) == wildcard).boxed()
.collect(Collectors.toList());
StringBuffer buff = new StringBuffer();
if (wildcardIndexes.isEmpty()) {
buff.append(LdapEncoder.filterEncode(searchString));
} else {
int previousWildcardIndex = -1;
for (int i = 0; i <= wildcardIndexes.size() - 1; i++) {
int wildcardIndex = wildcardIndexes.get(i);
if (wildcardIndex > previousWildcardIndex + 1 ) {
String substring = searchString.substring(previousWildcardIndex + 1, wildcardIndex);
buff.append(LdapEncoder.filterEncode(substring));
}
buff.append(wildcard);
// last wildcard in the string but not trailing
if (i == wildcardIndexes.size() - 1 && wildcardIndex < searchString.length() - 1) {
String substring = searchString.substring(wildcardIndex + 1, searchString.length());
buff.append(LdapEncoder.filterEncode(substring));
}
previousWildcardIndex = wildcardIndex;
}
}

return buff.toString();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't this all be replaced with:

List<String> tokens = StringUtils.split(searchString, wildcard);
List<String> encodedTokens = tokens.replaceAll(LdapEncoder::filterEncode);
return StringUtils.join(encodedTokens, wildcard);

Copy link
Contributor Author

@ssuvorov-fls ssuvorov-fls May 24, 2023

Choose a reason for hiding this comment

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

Hi
I've checked it and it doesn't work as expected
For * it returns empty string
For user* it returns user
For user*cn* it returns user*cn
My code will return correct string without any limitations of wildcard position

Copy link
Collaborator

@chrisknoll chrisknoll May 24, 2023

Choose a reason for hiding this comment

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

Understood, I didn't think through the corner cases, but your implementation is pretty complicated and I think I can modify the suggested with a few tweaks to hit the corner cases:

if (searchString == null || searchString.isEmpty() || wildCard.equals(searchString)) return searchString; // nothing to encode

List<String> tokens = StringUtils.split(searchString, wildcard);
List<String> encodedTokens = tokens.replaceAll(LdapEncoder::filterEncode);
String encodedSearchString = StringUtils.join(encodedTokens, wildcard) + searchString.endsWith(wildcard) ? wildcard : "";
return encodedSearchString;

I should note, my implementation will handle cases where wild card length is > 1. Not sure if that is the case with LDAP filtering, but it's good to be flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree
Your code is simpler
I made additional check for case when wildcard is at the first position

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for fixing that, it's hard doing this without having a compiler in front of me :) Yes, you captured the case where it might be like *user*, and the splitting of that just becomes user (which is the term we need to encode) but I lost the leading-wildcard that we need to check at the end when we re-assemble. Thank you for making this change.

@chrisknoll chrisknoll merged commit bb5d12d into master May 24, 2023
@delete-merged-branch delete-merged-branch bot deleted the issue-1790-fix-wildcard-ldap branch May 24, 2023 15:12
pieterlukasse pushed a commit to pieterlukasse/WebAPI that referenced this pull request Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Whitespace wildcards in AD group search
2 participants