♻️[#114] refactor setup configuration step

* Explicitly use model values
* Rename create_missing_groups to get_groups_by_name
* Move sync_groups condition to get_groups_by_name
* Fix sync_group doing nothing in step
* Test sync_group & sync_groups_glob_pattern
* Test idempotent and overwrite

mozilla_django_oidc_db/backends.py

@@ -24,7 +24,7 @@
 from .jwt import verify_and_decode_token
 from .models import OpenIDConnectConfigBase, UserInformationClaimsSources
 from .typing import ClaimPath, JSONObject
-from .utils import create_missing_groups, extract_content_type, obfuscate_claims
+from .utils import extract_content_type, get_groups_by_name, obfuscate_claims
 
 logger = logging.getLogger(__name__)
 
@@ -384,10 +384,9 @@ def _set_user_groups(
         return
 
     # Create missing groups if required
-    if sync_missing_groups:
-        existing_groups = create_missing_groups(desired_group_names, sync_groups_glob)
-    else:
-        existing_groups = set(Group.objects.filter(name__in=desired_group_names))
+    existing_groups = get_groups_by_name(
+        desired_group_names, sync_groups_glob, sync_missing_groups
+    )
 
     # at this point, existing_groups is the full collection of groups that should be
     # set on the user model, because:

mozilla_django_oidc_db/setup_configuration/steps.py

@@ -5,8 +5,9 @@
 from mozilla_django_oidc_db.models import OpenIDConnectConfig
 from mozilla_django_oidc_db.setup_configuration.models import (
     AdminOIDCConfigurationModel,
+    OIDCDiscoveryEndpoint,
 )
-from mozilla_django_oidc_db.utils import create_missing_groups
+from mozilla_django_oidc_db.utils import get_groups_by_name
 
 
 class AdminOIDCConfigurationStep(BaseConfigurationStep[AdminOIDCConfigurationModel]):
@@ -21,19 +22,46 @@ class AdminOIDCConfigurationStep(BaseConfigurationStep[AdminOIDCConfigurationMod
 
     def execute(self, model: AdminOIDCConfigurationModel) -> None:
 
-        config = OpenIDConnectConfig.get_solo()
+        all_settings = {
+            "enabled": model.enabled,
+            "oidc_rp_client_id": model.oidc_rp_client_id,
+            "oidc_rp_client_secret": model.oidc_rp_client_secret,
+            "oidc_rp_sign_algo": model.oidc_rp_sign_algo,
+            "oidc_rp_scopes_list": model.oidc_rp_scopes_list,
+            "oidc_op_jwks_endpoint": model.oidc_op_jwks_endpoint,
+            "oidc_token_use_basic_auth": model.oidc_token_use_basic_auth,
+            "oidc_rp_idp_sign_key": model.oidc_rp_idp_sign_key,
+            "oidc_op_logout_endpoint": model.oidc_op_logout_endpoint,
+            "oidc_use_nonce": model.oidc_use_nonce,
+            "oidc_nonce_size": model.oidc_nonce_size,
+            "oidc_state_size": model.oidc_state_size,
+            "oidc_keycloak_idp_hint": model.oidc_keycloak_idp_hint,
+            "userinfo_claims_source": model.userinfo_claims_source,
+            "username_claim": model.username_claim,
+            "claim_mapping": model.claim_mapping,
+            "groups_claim": model.groups_claim,
+            "sync_groups": model.sync_groups,
+            "sync_groups_glob_pattern": model.sync_groups_glob_pattern,
+            "make_users_staff": model.make_users_staff,
+            "superuser_group_names": model.superuser_group_names,
+            "default_groups": get_groups_by_name(
+                model.default_groups, model.sync_groups_glob_pattern, model.sync_groups
+            ),
+        }
 
-        all_settings = model.model_dump()
-        endpoint_config_data = all_settings.pop("endpoint_config")
-        all_settings.update(endpoint_config_data)
-
-        if groups := all_settings.get("default_groups"):
-            all_settings["default_groups"] = create_missing_groups(
-                groups, all_settings["sync_groups_glob_pattern"]
+        if isinstance(model.endpoint_config, OIDCDiscoveryEndpoint):
+            all_settings.update(
+                oidc_op_discovery_endpoint=model.endpoint_config.oidc_op_discovery_endpoint,
+            )
+        else:
+            all_settings.update(
+                oidc_op_authorization_endpoint=model.endpoint_config.oidc_op_authorization_endpoint,
+                oidc_op_token_endpoint=model.endpoint_config.oidc_op_token_endpoint,
+                oidc_op_user_endpoint=model.endpoint_config.oidc_op_user_endpoint,
             )
 
         form = OpenIDConnectConfigForm(
-            instance=config,
+            instance=OpenIDConnectConfig.get_solo(),
             data=all_settings,
         )
         if not form.is_valid():

mozilla_django_oidc_db/utils.py

@@ -94,13 +94,20 @@ def do_op_logout(config: OpenIDConnectConfigBase, id_token: str) -> None:
         )
 
 
-def create_missing_groups(
-    group_names: Iterable[str], sync_groups_glob: str = "*"
+def get_groups_by_name(
+    group_names: Iterable[str], sync_groups_glob: str, sync_missing_groups: bool
 ) -> set[Group]:
+    """
+    Gets Django User groups by name.
+
+    Optionally creates missing groups that match glob pattern.
+    """
 
     existing_groups = set(Group.objects.filter(name__in=group_names))
-    existing_group_names = {group.name for group in existing_groups}
+    if not sync_missing_groups:
+        return existing_groups
 
+    existing_group_names = {group.name for group in existing_groups}
     filtered_names = fnmatch.filter(
         set(group_names) - existing_group_names, sync_groups_glob
     )

tests/setupconfig/conftest.py

@@ -4,7 +4,7 @@
     OpenIDConnectConfig,
     UserInformationClaimsSources,
 )
-from mozilla_django_oidc_db.utils import create_missing_groups
+from mozilla_django_oidc_db.utils import get_groups_by_name
 
 """
 Key cloak credentials are setup for the keycloak docker-compose.yml.
@@ -33,6 +33,16 @@ def discovery_endpoint_config_yml():
     return "tests/setupconfig/files/discovery.yml"
 
 
+@pytest.fixture()
+def no_sync_groups_config_yml():
+    return "tests/setupconfig/files/no_sync_groups.yml"
+
+
+@pytest.fixture()
+def sync_groups_config_yml():
+    return "tests/setupconfig/files/sync_groups.yml"
+
+
 @pytest.fixture
 def set_config_to_non_default_values():
     """
@@ -42,8 +52,8 @@ def set_config_to_non_default_values():
     config = OpenIDConnectConfig.get_solo()
 
     # Will be always overwritten
-    config.oidc_rp_client_id = "client-id"
-    config.oidc_rp_client_secret = "secret"
+    config.oidc_rp_client_id = "different-client-id"
+    config.oidc_rp_client_secret = "different-secret"
     config.oidc_op_authorization_endpoint = "http://localhost:8080/whatever"
     config.oidc_op_token_endpoint = "http://localhost:8080/whatever"
     config.oidc_op_user_endpoint = "http://localhost:8080/whatever"
@@ -74,7 +84,7 @@ def set_config_to_non_default_values():
     config.oidc_state_size = 64
     config.userinfo_claims_source = UserInformationClaimsSources.userinfo_endpoint
 
-    config.default_groups.set(create_missing_groups(["OldAdmin", "OldUser"]))
+    config.default_groups.set(get_groups_by_name(["OldAdmin", "OldUser"], "*", True))
 
     config.save()
 

tests/setupconfig/files/no_sync_groups.yml

@@ -0,0 +1,12 @@
+oidc_db_config_enable: True
+oidc_db_config_admin_auth:
+  oidc_rp_client_id: client-id
+  oidc_rp_client_secret: secret
+  endpoint_config:
+    oidc_op_authorization_endpoint: http://localhost:8080/realms/test/protocol/openid-connect/auth
+    oidc_op_token_endpoint: http://localhost:8080/realms/test/protocol/openid-connect/token
+    oidc_op_user_endpoint: http://localhost:8080/realms/test/protocol/openid-connect/userinfo
+  sync_groups: false
+  default_groups:
+    - SuperAdmins
+    - NormalUsers

tests/setupconfig/files/sync_groups.yml

@@ -0,0 +1,15 @@
+oidc_db_config_enable: True
+oidc_db_config_admin_auth:
+  oidc_rp_client_id: client-id
+  oidc_rp_client_secret: secret
+  endpoint_config:
+    oidc_op_authorization_endpoint: http://localhost:8080/realms/test/protocol/openid-connect/auth
+    oidc_op_token_endpoint: http://localhost:8080/realms/test/protocol/openid-connect/token
+    oidc_op_user_endpoint: http://localhost:8080/realms/test/protocol/openid-connect/userinfo
+  sync_groups: true
+  sync_groups_glob_pattern: local.groups.*
+  default_groups:
+    - local.groups.SuperAdmins
+    - local.WeirdAdmins
+    - local.groups.NormalUsers
+    - local.WeirdUsers

tests/setupconfig/test_steps.py

@@ -1,13 +1,8 @@
-from io import StringIO
-
-from django.core.management import CommandError, call_command
+from django.contrib.auth.models import Group
 
 import pytest
 import requests
-from django_setup_configuration.exceptions import (
-    ConfigurationRunFailed,
-    PrerequisiteFailed,
-)
+from django_setup_configuration.exceptions import ConfigurationRunFailed
 from django_setup_configuration.test_utils import execute_single_step
 
 from mozilla_django_oidc_db.models import (
@@ -17,6 +12,7 @@
 from mozilla_django_oidc_db.setup_configuration.steps import AdminOIDCConfigurationStep
 
 from ..conftest import KEYCLOAK_BASE_URL
+from .conftest import set_config_to_non_default_values
 
 
 @pytest.fixture(autouse=True)
@@ -25,16 +21,17 @@ def clear_solo_cache():
     OpenIDConnectConfig.clear_cache()
 
 
-@pytest.mark.django_db
-def test_configure(full_config_yml):
-    execute_single_step(AdminOIDCConfigurationStep, yaml_source=full_config_yml)
-
+def assert_full_values():
     config = OpenIDConnectConfig.get_solo()
-
     assert not config.enabled
     assert config.oidc_rp_client_id == "client-id"
     assert config.oidc_rp_client_secret == "secret"
-    assert config.oidc_rp_scopes_list == ["open_id", "email", "profile", "extra_scope"]
+    assert config.oidc_rp_scopes_list == [
+        "open_id",
+        "email",
+        "profile",
+        "extra_scope",
+    ]
     assert config.oidc_rp_sign_algo == "RS256"
     assert config.oidc_rp_idp_sign_key == "key"
     assert config.oidc_op_discovery_endpoint == ""
@@ -71,6 +68,70 @@ def test_configure(full_config_yml):
     assert config.userinfo_claims_source == UserInformationClaimsSources.id_token
 
 
+@pytest.mark.django_db
+def test_configure_full(full_config_yml):
+
+    # create groups so they can be found
+    Group.objects.create(name="local.groups.Admins")
+    Group.objects.create(name="local.groups.Read-only")
+
+    # test if idempotent
+    execute_single_step(AdminOIDCConfigurationStep, yaml_source=full_config_yml)
+    assert_full_values()
+
+    execute_single_step(AdminOIDCConfigurationStep, yaml_source=full_config_yml)
+    assert_full_values()
+
+
+@pytest.mark.django_db
+def test_configure_overwrite(full_config_yml, set_config_to_non_default_values):
+
+    # create groups so they can be found
+    Group.objects.create(name="local.groups.Admins")
+    Group.objects.create(name="local.groups.Read-only")
+
+    config = OpenIDConnectConfig.get_solo()
+
+    # assert different values
+    assert not config.enabled
+    assert config.oidc_rp_client_id == "different-client-id"
+    assert config.oidc_rp_client_secret == "different-secret"
+    assert config.oidc_rp_scopes_list == [
+        "not_open_id",
+        "not_email",
+        "not_profile",
+        "not_extra_scope",
+    ]
+    assert config.oidc_rp_sign_algo == "M1911"
+    assert config.oidc_rp_idp_sign_key == "name"
+    assert config.oidc_op_discovery_endpoint == "http://localhost:8080/whatever"
+    assert config.oidc_op_jwks_endpoint == "http://localhost:8080/whatever"
+    assert config.oidc_op_authorization_endpoint == "http://localhost:8080/whatever"
+    assert config.oidc_op_token_endpoint == "http://localhost:8080/whatever"
+    assert config.oidc_op_user_endpoint == "http://localhost:8080/whatever"
+    assert config.username_claim == ["claim_title"]
+    assert config.groups_claim == ["groups_claim_title"]
+    assert config.claim_mapping == {"first_title": ["given_title"]}
+    assert config.sync_groups
+    assert config.sync_groups_glob_pattern == "not_local.groups.*"
+    assert set(group.name for group in config.default_groups.all()) == {
+        "OldAdmin",
+        "OldUser",
+    }
+    assert not config.make_users_staff
+    assert config.superuser_group_names == ["poweruser"]
+    assert config.oidc_use_nonce
+    assert config.oidc_nonce_size == 64
+    assert config.oidc_state_size == 64
+    assert (
+        config.userinfo_claims_source == UserInformationClaimsSources.userinfo_endpoint
+    )
+
+    execute_single_step(AdminOIDCConfigurationStep, yaml_source=full_config_yml)
+    # assert values overwritten
+    assert_full_values()
+
+
 @pytest.mark.django_db
 def test_configure_use_defaults(set_config_to_non_default_values, default_config_yml):
     execute_single_step(AdminOIDCConfigurationStep, yaml_source=default_config_yml)
@@ -181,3 +242,48 @@ def test_configure_discovery_failure(
     config = OpenIDConnectConfig.get_solo()
     assert not config.enabled
     assert config.oidc_op_discovery_endpoint == ""
+
+
+@pytest.mark.django_db
+def test_sync_groups_is_false(no_sync_groups_config_yml):
+    # create groups so they can be found
+    super_admin = Group.objects.create(name="SuperAdmins")
+
+    result = execute_single_step(
+        AdminOIDCConfigurationStep, yaml_source=no_sync_groups_config_yml
+    )
+
+    assert not result.config_model.sync_groups
+    assert result.config_model.default_groups == ["SuperAdmins", "NormalUsers"]
+
+    config = OpenIDConnectConfig.get_solo()
+    assert config.default_groups.all().count() == 1
+    assert config.default_groups.get() == super_admin
+
+
+@pytest.mark.django_db
+def test_sync_groups_is_true(sync_groups_config_yml):
+    # create groups so they can be found
+    super_admin = Group.objects.create(name="local.groups.SuperAdmins")
+    weird_admin = Group.objects.create(name="local.WeirdAdmins")
+
+    result = execute_single_step(
+        AdminOIDCConfigurationStep, yaml_source=sync_groups_config_yml
+    )
+
+    assert result.config_model.sync_groups
+    assert result.config_model.default_groups == [
+        "local.groups.SuperAdmins",
+        "local.WeirdAdmins",
+        "local.groups.NormalUsers",
+        "local.WeirdUsers",
+    ]
+    assert result.config_model.sync_groups_glob_pattern == "local.groups.*"
+
+    config = OpenIDConnectConfig.get_solo()
+    assert config.default_groups.all().count() == 3
+    assert super_admin in config.default_groups.all()
+    assert weird_admin in config.default_groups.all()
+    assert config.default_groups.all().filter(name="local.groups.NormalUsers").exists()
+    # Does not match glob, is not created
+    assert not config.default_groups.all().filter(name="local.WeirdUsers").exists()