I also wanted to add that i'm on vacation for three weeks from 14.08 until 5.09. I won't be able to respond during that time.

Is there any chance this change can be merged? I also have this issue searching for users.

@vramik Thank you for your response. I just rebased the branch and it's up to date with master. But somehow there is still a merge conflict displayed. Maybe it just needs time to synchronize.

@artur-baltabayev I believe that it has been synced with master. Keycloak's main branch is now called main which causes showing the conflicts. Could you please rebase to the latest main?

I've started discussion regarding the change of the default behavior: #9507

@hmlnarik Thanks for the hint. Now it's rebased on the main branch,

@hmlnarik Thanks for the hint. Now it's rebased on the main branch,

Thank you @artur-baltabayev, it seems there has happened some issue as the branch you're sending the PR from seems to be 118 commits behind keycloak:main, could you please double-check it?

@vramik Implemented your feedback. I also removed some imports that were unused in the test classes.

@vramik Ok i remember now that the single wildcard workaround was needed for undertow. As you can see, currently every undertow test that searches for a empty string fails. That is because it defaults to a 'empty' prefix search which is represented by a single wildcard character (i.e. '%'). We couldn't come up with another fix for this problem, so i would appreciate suggestions. Otherwise i can add the workaround again.

Thank you @artur-baltabayev. Please correct me if I'm wrong, but I think search for empty string is equivalent to getting all users. In that case it could be checked in UsersResource e.g. following way:

if (search != null && ! search.trim().isEmpty())

@vramik You mean we should move the workaround to the UsersResource instead of being MapProvider specific ?

@vramik You mean we should move the workaround to the UsersResource instead of being MapProvider specific ?

I don't think there should be added any kind of workaround. When you asked for suggestions regarding the empty string searches I've realized that empty string search should be considered as equal to getting all users.

It should work for both legacy-jpa and map-storage, why exactly it is failing for map storage and not for legacy-jpa?

@vramik My observation was that the test cases failed after i removed the workaround. But since you were pointing out (https://issues.redhat.com/browse/KEYCLOAK-18969), that the mentioned single wildcard issue no longer exists i will have to do some further testing before i can answer you question.

Hello @vramik , after i did some more research i can now explain why you couldn't recreate the mentioned single wildcard issue (https://issues.redhat.com/browse/KEYCLOAK-18969) and why it's failing in the first place :

You can only recreate the issue on this branch (not on main) since it is tied to how the new user search works. I recreated it locally by running the following command : mvn clean install -nsu -B -Pauth-server-undertow -Pmap-storage -Dtest=UserTest -Dkeycloak.logging.level=info -f testsuite/integration-arquillian/tests/base/pom.xml .

The error is map storage specific because it comes from the 'like' and 'ilike' methods in the CriteriaOperator class. There is a substring call that wasn't made for input strings that only contain one character (in our case it's the single wildcard).
Here is the actual code snippet (affected logic is similar between the 'like' and 'ilike' method) :

public static Predicate<Object> ilike(Object[] value) {
    Object value0 = getFirstArrayElement(value);
    if (value0 instanceof String) {
        String sValue = (String) value0;
        boolean anyBeginning = sValue.startsWith("%");
        boolean anyEnd = sValue.endsWith("%");

        Pattern pValue = Pattern.compile(
          (anyBeginning ? ".*" : "")
          + Pattern.quote(sValue.substring(anyBeginning ? 1 : 0, sValue.length() - (anyEnd ? 1 : 0)))
          + (anyEnd ? ".*" : ""),
          Pattern.CASE_INSENSITIVE + Pattern.DOTALL
        );
        return o -> {
            return o instanceof String && pValue.matcher((String) o).matches();
        };
    }
    return ALWAYS_FALSE;
}

If value0 equals to %, both anyBeginning and anyEnd will be evaluated to true which leads to a substring(1, 0) call. This produces a java.lang.StringIndexOutOfBoundsException: String index out of range: -1 exception. Now considering all search queries originally started and ended with a wildcard, there wasn't a need to do a corner case check for a single character string. But now every search without a asterisk (*) in it defaults to a prefix search, and prefix searches are implemented to end with a single wildcard. This means that a empty search string will get translated to a single wildcard character. I hope the issue is clearer now.

I have several suggestions to fix this issue :

1. Going back to the old map storage specific fix.
2. Defaulting every empty search string to a double wildcard string instead of just the map storage.
3. Fixing the issue in the map storage directly (maybe it could be part of this PR ?).

Option 3 please. If value0 consists only of % characters (i.e. matches ^%+$ regex), the predicate should be just returning true unconditionally.

@hmlnarik @vramik Implemented the fix in the CriteriaOperator class. Now every string that matches the suggested pattern will be evaluated to true.

@artur-baltabayev Thank you for the code changes, they now look good to me. Could you please prepare a PR for release notes in https://github.com/keycloak/keycloak-documentation/blob/main/release_notes/topics for version 17 (if not present, please introduce file 17_0_0.adoc)? The note in the release notes should cover the information stated in the description of this PR.

@vramik @hmlnarik Thank you two a lot for all the feedback! I just made the documentation PR : keycloak/keycloak-documentation#1378 .

Just out of curiosity using % in the search parameter instead of * will produce the infix/prefix search result. Is it wanted?

Good improvement. Just one question why it doesn't include the case *value?

Good improvement. Just one question why it doesn't include the case *value?

It's maybe not that intuitive, but you should be able to handle that case like that: "%value"