Fix provider name handling in OIDC integration

This fixes a problem in the OIDC middleware where secondary providers with lowercase names were not found in the OIDC_PROVIDERS setting.
artefactual · Oct 10, 2024 · 6181b9d · 6181b9d
1 parent a27a9f6
commit 6181b9d
Show file tree

Hide file tree

Showing 6 changed files with 125 additions and 45 deletions.
diff --git a/src/archivematicaCommon/lib/archivematicaFunctions.py b/src/archivematicaCommon/lib/archivematicaFunctions.py
@@ -29,6 +29,8 @@
 import re
 from itertools import zip_longest
 from pathlib import Path
+from typing import Dict
+from typing import Iterable
 from uuid import uuid4
 
 from amclient import AMClient
@@ -524,3 +526,40 @@ def chunk_iterable(iterable, chunk_size=10, fillvalue=None):
     """
     args = [iter(iterable)] * chunk_size
     return zip_longest(*args, fillvalue=fillvalue)
+
+
+def get_oidc_secondary_providers(
+    oidc_secondary_provider_names: Iterable[str],
+) -> Dict[str, Dict[str, str]]:
+    """Build secondary OIDC provider details dict. Takes a list of secondary
+    OIDC providers and gathers details about these providers from env vars.
+    Output dict contains details for each OIDC connection which can then be
+    referenced by name.
+    """
+
+    providers = {}
+
+    for provider_name in oidc_secondary_provider_names:
+        provider_name = provider_name.strip().upper()
+        client_id = os.environ.get(f"OIDC_RP_CLIENT_ID_{provider_name}")
+        client_secret = os.environ.get(f"OIDC_RP_CLIENT_SECRET_{provider_name}")
+        authorization_endpoint = os.environ.get(
+            f"OIDC_OP_AUTHORIZATION_ENDPOINT_{provider_name}", ""
+        )
+        token_endpoint = os.environ.get(f"OIDC_OP_TOKEN_ENDPOINT_{provider_name}", "")
+        user_endpoint = os.environ.get(f"OIDC_OP_USER_ENDPOINT_{provider_name}", "")
+        jwks_endpoint = os.environ.get(f"OIDC_OP_JWKS_ENDPOINT_{provider_name}", "")
+        logout_endpoint = os.environ.get(f"OIDC_OP_LOGOUT_ENDPOINT_{provider_name}", "")
+
+        if client_id and client_secret:
+            providers[provider_name] = {
+                "OIDC_RP_CLIENT_ID": client_id,
+                "OIDC_RP_CLIENT_SECRET": client_secret,
+                "OIDC_OP_AUTHORIZATION_ENDPOINT": authorization_endpoint,
+                "OIDC_OP_TOKEN_ENDPOINT": token_endpoint,
+                "OIDC_OP_USER_ENDPOINT": user_endpoint,
+                "OIDC_OP_JWKS_ENDPOINT": jwks_endpoint,
+                "OIDC_OP_LOGOUT_ENDPOINT": logout_endpoint,
+            }
+
+    return providers
diff --git a/src/dashboard/src/components/accounts/backends.py b/src/dashboard/src/components/accounts/backends.py
@@ -59,10 +59,7 @@ def get_settings(self, attr, *args):
             if request:
                 provider_name = request.session.get("providername")
 
-                if (
-                    provider_name
-                    and provider_name in settings.OIDC_SECONDARY_PROVIDER_NAMES
-                ):
+                if provider_name and provider_name in settings.OIDC_PROVIDERS:
                     provider_settings = settings.OIDC_PROVIDERS.get(provider_name, {})
                     value = provider_settings.get(attr)
 

diff --git a/src/dashboard/src/components/accounts/views.py b/src/dashboard/src/components/accounts/views.py
@@ -222,10 +222,7 @@ def get_settings(self, attr, *args):
             if request:
                 provider_name = request.session.get("providername")
 
-                if (
-                    provider_name
-                    and provider_name in settings.OIDC_SECONDARY_PROVIDER_NAMES
-                ):
+                if provider_name and provider_name in settings.OIDC_PROVIDERS:
                     provider_settings = settings.OIDC_PROVIDERS.get(provider_name, {})
                     value = provider_settings.get(attr)
 
@@ -286,7 +283,7 @@ def get_oidc_logout_url(request):
     if request:
         provider_name = request.session.get("providername")
 
-        if provider_name and provider_name in settings.OIDC_SECONDARY_PROVIDER_NAMES:
+        if provider_name and provider_name in settings.OIDC_PROVIDERS:
             provider_settings = settings.OIDC_PROVIDERS.get(provider_name, {})
             end_session_endpoint = provider_settings.get("OIDC_OP_LOGOUT_ENDPOINT")
 

diff --git a/src/dashboard/src/settings/components/oidc_auth.py b/src/dashboard/src/settings/components/oidc_auth.py
@@ -1,40 +1,6 @@
 import os
 
-
-def get_oidc_secondary_providers(oidc_secondary_provider_names):
-    """Build secondary OIDC provider details dict. Takes a list of secondary
-    OIDC providers and gathers details about these providers from env vars.
-    Output dict contains details for each OIDC connection which can then be
-    referenced by name.
-    """
-
-    providers = {}
-
-    for provider_name in oidc_secondary_provider_names:
-        provider_name = provider_name.strip()
-        client_id = os.environ.get(f"OIDC_RP_CLIENT_ID_{provider_name}")
-        client_secret = os.environ.get(f"OIDC_RP_CLIENT_SECRET_{provider_name}")
-        authorization_endpoint = os.environ.get(
-            f"OIDC_OP_AUTHORIZATION_ENDPOINT_{provider_name}", ""
-        )
-        token_endpoint = os.environ.get(f"OIDC_OP_TOKEN_ENDPOINT_{provider_name}", "")
-        user_endpoint = os.environ.get(f"OIDC_OP_USER_ENDPOINT_{provider_name}", "")
-        jwks_endpoint = os.environ.get(f"OIDC_OP_JWKS_ENDPOINT_{provider_name}", "")
-        logout_endpoint = os.environ.get(f"OIDC_OP_LOGOUT_ENDPOINT_{provider_name}", "")
-
-        if client_id and client_secret:
-            providers[provider_name] = {
-                "OIDC_RP_CLIENT_ID": client_id,
-                "OIDC_RP_CLIENT_SECRET": client_secret,
-                "OIDC_OP_AUTHORIZATION_ENDPOINT": authorization_endpoint,
-                "OIDC_OP_TOKEN_ENDPOINT": token_endpoint,
-                "OIDC_OP_USER_ENDPOINT": user_endpoint,
-                "OIDC_OP_JWKS_ENDPOINT": jwks_endpoint,
-                "OIDC_OP_LOGOUT_ENDPOINT": logout_endpoint,
-            }
-
-    return providers
-
+from archivematicaFunctions import get_oidc_secondary_providers
 
 OIDC_RP_CLIENT_ID = os.environ.get("OIDC_RP_CLIENT_ID", "")
 OIDC_RP_CLIENT_SECRET = os.environ.get("OIDC_RP_CLIENT_SECRET", "")

diff --git a/tests/archivematicaCommon/test_archivematica_functions.py b/tests/archivematicaCommon/test_archivematica_functions.py
@@ -105,3 +105,84 @@ def test_package_name_from_path():
             current_path, remove_uuid_suffix=True
         )
         assert package_name_without_uuid == test_package["package_name_without_uuid"]
+
+
+def test_get_oidc_secondary_providers_ignores_provider_if_client_id_and_secret_are_missing(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    monkeypatch.setenv("OIDC_RP_CLIENT_ID_FOO", "foo-client-id")
+    monkeypatch.setenv("OIDC_RP_CLIENT_SECRET_FOO", "foo-client-secret")
+    monkeypatch.setenv("OIDC_RP_CLIENT_ID_BAR", "bar-client-id")
+    monkeypatch.setenv("OIDC_RP_CLIENT_SECRET_BAZ", "foo-secret")
+
+    assert am.get_oidc_secondary_providers(["FOO", "BAR", "BAZ"]) == {
+        "FOO": {
+            "OIDC_OP_AUTHORIZATION_ENDPOINT": "",
+            "OIDC_OP_JWKS_ENDPOINT": "",
+            "OIDC_OP_LOGOUT_ENDPOINT": "",
+            "OIDC_OP_TOKEN_ENDPOINT": "",
+            "OIDC_OP_USER_ENDPOINT": "",
+            "OIDC_RP_CLIENT_ID": "foo-client-id",
+            "OIDC_RP_CLIENT_SECRET": "foo-client-secret",
+        }
+    }
+
+
+def test_get_oidc_secondary_providers_strips_provider_names(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    monkeypatch.setenv("OIDC_RP_CLIENT_ID_FOO", "foo-client-id")
+    monkeypatch.setenv("OIDC_RP_CLIENT_SECRET_FOO", "foo-client-secret")
+    monkeypatch.setenv("OIDC_RP_CLIENT_ID_BAR", "bar-client-id")
+    monkeypatch.setenv("OIDC_RP_CLIENT_SECRET_BAR", "bar-client-secret")
+
+    assert am.get_oidc_secondary_providers(["  FOO", " BAR  "]) == {
+        "FOO": {
+            "OIDC_OP_AUTHORIZATION_ENDPOINT": "",
+            "OIDC_OP_JWKS_ENDPOINT": "",
+            "OIDC_OP_LOGOUT_ENDPOINT": "",
+            "OIDC_OP_TOKEN_ENDPOINT": "",
+            "OIDC_OP_USER_ENDPOINT": "",
+            "OIDC_RP_CLIENT_ID": "foo-client-id",
+            "OIDC_RP_CLIENT_SECRET": "foo-client-secret",
+        },
+        "BAR": {
+            "OIDC_OP_AUTHORIZATION_ENDPOINT": "",
+            "OIDC_OP_JWKS_ENDPOINT": "",
+            "OIDC_OP_LOGOUT_ENDPOINT": "",
+            "OIDC_OP_TOKEN_ENDPOINT": "",
+            "OIDC_OP_USER_ENDPOINT": "",
+            "OIDC_RP_CLIENT_ID": "bar-client-id",
+            "OIDC_RP_CLIENT_SECRET": "bar-client-secret",
+        },
+    }
+
+
+def test_get_oidc_secondary_providers_capitalizes_provider_names(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    monkeypatch.setenv("OIDC_RP_CLIENT_ID_FOO", "foo-client-id")
+    monkeypatch.setenv("OIDC_RP_CLIENT_SECRET_FOO", "foo-client-secret")
+    monkeypatch.setenv("OIDC_RP_CLIENT_ID_BAR", "bar-client-id")
+    monkeypatch.setenv("OIDC_RP_CLIENT_SECRET_BAR", "bar-client-secret")
+
+    assert am.get_oidc_secondary_providers(["fOo", "bar"]) == {
+        "FOO": {
+            "OIDC_OP_AUTHORIZATION_ENDPOINT": "",
+            "OIDC_OP_JWKS_ENDPOINT": "",
+            "OIDC_OP_LOGOUT_ENDPOINT": "",
+            "OIDC_OP_TOKEN_ENDPOINT": "",
+            "OIDC_OP_USER_ENDPOINT": "",
+            "OIDC_RP_CLIENT_ID": "foo-client-id",
+            "OIDC_RP_CLIENT_SECRET": "foo-client-secret",
+        },
+        "BAR": {
+            "OIDC_OP_AUTHORIZATION_ENDPOINT": "",
+            "OIDC_OP_JWKS_ENDPOINT": "",
+            "OIDC_OP_LOGOUT_ENDPOINT": "",
+            "OIDC_OP_TOKEN_ENDPOINT": "",
+            "OIDC_OP_USER_ENDPOINT": "",
+            "OIDC_RP_CLIENT_ID": "bar-client-id",
+            "OIDC_RP_CLIENT_SECRET": "bar-client-secret",
+        },
+    }
diff --git a/tests/integration/docker-compose.yml b/tests/integration/docker-compose.yml
@@ -61,7 +61,7 @@ services:
       OIDC_OP_USER_ENDPOINT: "http://keycloak:8080/realms/demo/protocol/openid-connect/userinfo"
       OIDC_OP_JWKS_ENDPOINT: "http://keycloak:8080/realms/demo/protocol/openid-connect/certs"
       OIDC_OP_LOGOUT_ENDPOINT: "http://keycloak:8080/realms/demo/protocol/openid-connect/logout"
-      OIDC_SECONDARY_PROVIDER_NAMES: "SECONDARY"
+      OIDC_SECONDARY_PROVIDER_NAMES: "secondary"
       OIDC_RP_CLIENT_ID_SECONDARY: "am-dashboard-secondary"
       OIDC_RP_CLIENT_SECRET_SECONDARY: "example-secret-secondary"
       OIDC_OP_AUTHORIZATION_ENDPOINT_SECONDARY: "http://keycloak:8080/realms/secondary/protocol/openid-connect/auth"