keycloak 26.0.6 compatibility (#167)

* switch to keycloak 26.0.6

* <bot> update dependencies*.log files(s)

* update list_users query handling for 26.0.6

* make flake8 happy

* comment update

---------

Co-authored-by: github-actions <[email protected]>

dependencies-from-Dockerfile.log

@@ -46,7 +46,7 @@ requests==2.32.3
 requests-futures==1.0.2
 requests-oauthlib==2.0.0
 rsa==4.9
-tornado==6.4.1
+tornado==6.4.2
 typing_extensions==4.12.2
 Unidecode==1.3.8
 uritemplate==4.1.1
@@ -169,7 +169,7 @@ wipac-keycloak-rest-services
     │       ├── charset-normalizer [required: >=2,<4, installed: 3.4.0]
     │       ├── idna [required: >=2.5,<4, installed: 3.10]
     │       └── urllib3 [required: >=1.21.1,<3, installed: 2.2.3]
-    ├── tornado [required: Any, installed: 6.4.1]
+    ├── tornado [required: Any, installed: 6.4.2]
     ├── urllib3 [required: >=2.0.4, installed: 2.2.3]
     └── wipac-dev-tools [required: Any, installed: 1.13.0]
         ├── requests [required: Any, installed: 2.32.3]

krs/users.py

@@ -49,19 +49,16 @@ async def list_users(search=None, attr_query=None, rest_client=None):
         # As of KeyCloak 24, the q parameter is ignored if search is specified
         raise ValueError("Parameters search and query are mutually exclusive")
 
-    # Validate and if necessary/possible make the queries suitable for embedding in URL
+    # Do basic query validation. This will reject possibly valid queries if
+    # I am not confident that I can format them correctly in the URL.
+    bad_chars = set('&"')
     if attr_query:
-        for key, value in attr_query.copy().items():
-            if set('&"\'') & (set(str(value)) | set(str(key))):
-                raise NotImplementedError(f"Not yet capable of encoding attribute query {attr_query}")
-            if ' ' in str(value):
-                attr_query[key] = f'"{value}"'
-            if ' ' in str(key) or ':' in str(key):
-                new_key = f'"{key}"'
-                if new_key in attr_query:
-                    raise NotImplementedError(f"Not yet capable of encoding attribute {attr_query}")
-                attr_query[new_key] = attr_query[key]
-                attr_query.pop(key)
+        for key, value in attr_query.items():
+            if (bad_chars & (set(str(value)) | set(str(key)))
+                    or ':' in str(key)):
+                raise NotImplementedError(f"Cowardly refusing to run query {dict([(key,value)])}."
+                                          f" Either I don't know how to format the query or"
+                                          f" Keycloak is known to have trouble with similar queries.")
 
     inc = 50
     ret = {}
@@ -72,7 +69,8 @@ async def list_users(search=None, attr_query=None, rest_client=None):
             url += f'&search={search}'
         if attr_query:
             # query format here: https://www.keycloak.org/docs-api/24.0.1/rest-api/index.html
-            url += "&q=" + " ".join(f"{key}:{val}" for key, val in attr_query.items())
+            # double quote everything to be safe
+            url += "&q=" + " ".join(f'"{key}":"{val}"' for key, val in attr_query.items())
         data = await rest_client.request('GET', url)
         for u in data:
             fix_singleton_attributes(u)

resources/keycloak-image/Dockerfile

@@ -1,4 +1,4 @@
-FROM quay.io/keycloak/keycloak:26.0.5 as builder
+FROM quay.io/keycloak/keycloak:26.0.6 as builder
 
 # Keycloak core settings
 ENV KC_HTTP_ENABLED="true"

tests/krs/test_users.py

@@ -23,6 +23,25 @@ async def test_list_users(keycloak_bootstrap):
     assert ret['testuser']['email'] == 'foo@test'
 
 
+# krs.users.list_users()'s query validation is based in part on assumptions about
+# version-specific behavior of Keycloak. Some of this assumed behavior is problematic
+# from the operational standpoint and could be result of bugs that prevent certain
+# valid attribute keys/values to be queried using the /users q= API. The purpose of
+# this test is to confirm out-of-band that the *reasonable* queries blocked by the
+# validation code of krs.users.list_users() indeed don't work correctly with the
+# version of Keycloak being tested. If this test fails, krs.users.list_users()'s
+# query *validation* logic needs to be updated to allow the type of query in question
+# (proper handling of the query should be tested elsewhere)
+# noinspection LongLine
+@pytest.mark.asyncio
+async def test_important_user_attr_query_quirks(keycloak_bootstrap):
+    await users.create_user('user', first_name='f', last_name='l', email='user@test',
+                            attribs={'colon:in:key': 1}, rest_client=keycloak_bootstrap)
+    # Query format buried here: https://www.keycloak.org/docs-api/26.0.6/rest-api/index.html
+    res = await keycloak_bootstrap.request('GET', '/users?q="colon:in:key":1')
+    assert not res
+
+
 # noinspection LongLine
 @pytest.mark.asyncio
 async def test_list_user_attr_query_simple(keycloak_bootstrap):
@@ -37,34 +56,67 @@ async def test_list_user_attr_query_simple(keycloak_bootstrap):
     assert sorted(ret.keys()) == ['mult_attrs_match1', 'mult_attrs_match2']
 
 
+# Construct "tricky" user attribute query keys and values that are valid,
+# but need to be handled with care by krs.users.list_users() (simple cases
+# should be tested from test_list_user_attr_query_simple(). The intention
+# here is not to be exhaustive and try to detect every minute change in
+# behavior between Keycloak's versions, but to only test for edge cases
+# that are reasonable (e.g. as of Keycloak 24.0.3 "  " is a valid key, but
+# don't test for it because using such a key is not super smart).
+valid_tricky_attr_query_keys = [' beginning_with_space',
+                                'ending_with_space ',
+                                'containing space',
+                                "'single-quoted string'"]
+valid_tricky_attr_query_values = ['a:b may_cause_ambiguity'] + valid_tricky_attr_query_keys
+
+
 # noinspection LongLine
+@pytest.mark.parametrize('key', valid_tricky_attr_query_keys)
 @pytest.mark.asyncio
-async def test_list_user_attr_query_transforms(keycloak_bootstrap):
-    await users.create_user('spaces', first_name='f', last_name='l', email='spaces@test',
-                            attribs={'_ _': '_ _'}, rest_client=keycloak_bootstrap)
-    ret = await users.list_users(attr_query={'_ _': '_ _'}, rest_client=keycloak_bootstrap)
-    assert sorted(ret.keys()) == ['spaces']
+async def test_list_user_attr_query_tricky_valid_key(key, keycloak_bootstrap):
+    await users.create_user('user', first_name='f', last_name='l', email='user@test',
+                            attribs={key: 'value'}, rest_client=keycloak_bootstrap)
+    ret = await users.list_users(attr_query={key: 'value'}, rest_client=keycloak_bootstrap)
+    assert list(ret.keys()) == ['user']
 
-    await users.create_user('colons', first_name='f', last_name='l', email='colons@test',
-                            attribs={':': ':'}, rest_client=keycloak_bootstrap)
-    ret = await users.list_users(attr_query={':': ':'}, rest_client=keycloak_bootstrap)
-    assert sorted(ret.keys()) == ['colons']
+
+# noinspection LongLine
+@pytest.mark.parametrize('value', valid_tricky_attr_query_values)
+@pytest.mark.asyncio
+async def test_list_user_attr_query_tricky_valid_value(value, keycloak_bootstrap):
+    await users.create_user('user', first_name='f', last_name='l', email='user@test',
+                            attribs={'key': value}, rest_client=keycloak_bootstrap)
+    ret = await users.list_users(attr_query={'key': value}, rest_client=keycloak_bootstrap)
+    assert list(ret.keys()) == ['user']
+
+
+# Construct user attribute query keys and values that are either inherently
+# invalid, or may not be formatted correctly by krs.users.list_users(), or are
+# handled inconsistently by different versions of Keycloak, and so should be
+# rejected by krs.users.list_users(). Only test for reasonable cases. If you
+# use "  " as a value, there is no helping you.
+problematic_attr_query_keys = [':a', 'a&a', 'a"a']
+problematic_attr_query_values = ['a&a', 'a"a']
 
 
 # noinspection LongLine
+@pytest.mark.parametrize('key', problematic_attr_query_keys)
 @pytest.mark.asyncio
-async def test_list_user_attr_query_invalid(keycloak_bootstrap):
-    bad_chars = "&'\""
-    for i, char in enumerate(bad_chars):
-        await users.create_user(f'attr_val_not_impl{i}', first_name='f', last_name='l', email=f'val-not-impl{i}@test',
-                                attribs={'not_impl_val': f"{char}"}, rest_client=keycloak_bootstrap)
+async def test_list_user_attr_query_problematic_keys(key, keycloak_bootstrap):
+        await users.create_user(f'bad', first_name='f', last_name='l', email=f'bad@test',
+                                attribs={key: "value"}, rest_client=keycloak_bootstrap)
         with pytest.raises(NotImplementedError):
-            assert not await users.list_users(attr_query={'not_impl_val': f"{char}"}, rest_client=keycloak_bootstrap)
+            assert not await users.list_users(attr_query={key: "value"}, rest_client=keycloak_bootstrap)
 
-        await users.create_user(f'attr_name_not_impl{i}', first_name='f', last_name='l', email=f'name-not-impl{i}@test',
-                                attribs={f'{char}': 'not_impl_name'}, rest_client=keycloak_bootstrap)
-        with pytest.raises(NotImplementedError):
-            assert not await users.list_users(attr_query={f"{char}": "not_impl_name"}, rest_client=keycloak_bootstrap)
+
+# noinspection LongLine
+@pytest.mark.parametrize('value', problematic_attr_query_values)
+@pytest.mark.asyncio
+async def test_list_user_attr_query_problematic_values(value, keycloak_bootstrap):
+    await users.create_user(f'bad', first_name='f', last_name='l', email=f'bad@test',
+                            attribs={"key": value}, rest_client=keycloak_bootstrap)
+    with pytest.raises(NotImplementedError):
+        assert not await users.list_users(attr_query={"key": value}, rest_client=keycloak_bootstrap)
 
 
 # noinspection LongLine