DM-48326: Avoid commas in generated auth-url annotation

changelog.d/20250108_223210_rra_DM_48326.md

@@ -0,0 +1,3 @@
+### Bug fixes
+
+- Pass multiple delegate scopes to the `/auth` route by repeating the `delegate_scope` query parameter instead of passing a comma-separated list as a single value. ingress-nginx 4.12.0 no longer allows `%` in the `auth-url` annotation and `,` was not initially allowed, and this matches how `scope` was handled.

src/gafaelfawr/handlers/ingress.py

@@ -128,15 +128,14 @@ def auth_config(
         ),
     ] = None,
     delegate_scope: Annotated[
-        str | None,
+        list[str] | None,
         Query(
             title="Scope of delegated token",
             description=(
-                "Comma-separated list of scopes to add to the delegated token."
-                " All listed scopes are implicitly added to the scope"
-                " requirements for authorization."
+                "Scopes to add to the delegated token if present in the"
+                " token used for authentication"
             ),
-            examples=["read:all,write:all"],
+            examples=[["read:all", "write:all"]],
         ),
     ] = None,
     minimum_lifetime: Annotated[
@@ -195,7 +194,7 @@ def auth_config(
                 "If given more than once, meaning is determined by the"
                 " `satisfy` parameter"
             ),
-            examples=["read:all"],
+            examples=[["read:all"]],
         ),
     ] = None,
     service: Annotated[
@@ -263,15 +262,11 @@ def auth_config(
     if username:
         context.rebind_logger(required_user=username)
 
-    if delegate_scope:
-        delegate_scopes = {s.strip() for s in delegate_scope.split(",")}
-    else:
-        delegate_scopes = set()
     if not minimum_lifetime and (notebook or delegate_to):
         minimum_lifetime = MINIMUM_LIFETIME
     return AuthConfig(
         auth_type=auth_type,
-        delegate_scopes=delegate_scopes,
+        delegate_scopes=set(delegate_scope) if delegate_scope else set(),
         delegate_to=delegate_to,
         minimum_lifetime=minimum_lifetime,
         only_services=set(only_service) if only_service else None,

src/gafaelfawr/models/kubernetes.py

@@ -372,8 +372,10 @@ def to_auth_query(self) -> list[tuple[str, str]]:
             elif self.delegate.internal:
                 service = self.delegate.internal.service
                 query.append(("delegate_to", service))
-                scopes = ",".join(self.delegate.internal.scopes)
-                query.append(("delegate_scope", scopes))
+                query.extend(
+                    ("delegate_scope", s)
+                    for s in self.delegate.internal.scopes
+                )
             if self.delegate.minimum_lifetime:
                 minimum_lifetime = self.delegate.minimum_lifetime
                 minimum_str = str(int(minimum_lifetime.total_seconds()))

tests/data/kubernetes/output/ingresses.yaml

@@ -95,7 +95,7 @@ metadata:
   annotations:
     nginx.ingress.kubernetes.io/auth-method: GET
     nginx.ingress.kubernetes.io/auth-response-headers: "Authorization,Cookie,X-Auth-Request-Email,X-Auth-Request-Service,X-Auth-Request-Token,X-Auth-Request-User"
-    nginx.ingress.kubernetes.io/auth-url: "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth?scope=read:all&scope=read:some&delegate_to=some-service&delegate_scope=read:all%2Cread:some&auth_type=basic"
+    nginx.ingress.kubernetes.io/auth-url: "http://gafaelfawr.gafaelfawr.svc.cluster.local:8080/ingress/auth?scope=read:all&scope=read:some&delegate_to=some-service&delegate_scope=read:all&delegate_scope=read:some&auth_type=basic"
     nginx.ingress.kubernetes.io/configuration-snippet: |
       {snippet}
   creationTimestamp: {any}

tests/handlers/ingress_test.py

@@ -311,12 +311,13 @@ async def test_internal(client: AsyncClient, factory: Factory) -> None:
 
     r = await client.get(
         "/ingress/auth",
-        params={
-            "scope": "exec:admin",
-            "service": "a-service",
-            "delegate_to": "a-service",
-            "delegate_scope": " read:some  ,read:all  ",
-        },
+        params=[
+            ("scope", "exec:admin"),
+            ("service", "a-service"),
+            ("delegate_to", "a-service"),
+            ("delegate_scope", "read:some"),
+            ("delegate_scope", "read:all"),
+        ],
         headers={"Authorization": f"Bearer {token_data.token}"},
     )
     assert r.status_code == 200
@@ -365,11 +366,12 @@ async def test_internal(client: AsyncClient, factory: Factory) -> None:
     # Requesting a token with the same parameters returns the same token.
     r = await client.get(
         "/ingress/auth",
-        params={
-            "scope": "exec:admin",
-            "delegate_to": "a-service",
-            "delegate_scope": "read:all,read:some",
-        },
+        params=(
+            ("scope", "exec:admin"),
+            ("delegate_to", "a-service"),
+            ("delegate_scope", "read:all"),
+            ("delegate_scope", "read:some"),
+        ),
         headers={"Authorization": f"Bearer {token_data.token}"},
     )
     assert r.status_code == 200
@@ -384,11 +386,12 @@ async def test_internal_scopes(client: AsyncClient, factory: Factory) -> None:
 
     r = await client.get(
         "/ingress/auth",
-        params={
-            "scope": "read:some",
-            "delegate_to": "a-service",
-            "delegate_scope": "read:all,read:some",
-        },
+        params=(
+            ("scope", "read:some"),
+            ("delegate_to", "a-service"),
+            ("delegate_scope", "read:all"),
+            ("delegate_scope", "read:some"),
+        ),
         headers={"Authorization": f"Bearer {token_data.token}"},
     )
     assert r.status_code == 200