Repository: jupyterhub/oauthenticator (python)

Issue:
[CILogon] Let allowed_idps allow `default` to be configured
## Pivoted issue

In the original issue I describe a problem and proposed a solution to allow `allowed_idps` to be a list, but I think now its better to let `allowed_idps` support a `default` as motivated via https://github.com/jupyterhub/oauthenticator/issues/690#issuecomment-1759408005.

## Original issue

CILogon can be used with multiple identity providers (idps) behind it. We configure `allowed_idps` and then when redirecting to CILogon to login we pass a query string in the URL to limit the choices to those idps as well. The ordering the idp's listed in this query string influences the ordering of what is presented to users.

```
# this is the query string, with temple first, google second
selected_idp=https%3A%2F%2Ffim.temple.edu%2Fidp%2Fshibboleth%2Chttp%3A%2F%2Fgoogle.com%2Faccounts%2Fo8%2Fid
```

![ordering-of-idps](https://github.com/jupyterhub/oauthenticator/assets/3837114/d9457ea1-be23-43f7-b020-89dd4126d6f9)

In a z2jh deployment using CILogon our helm chart looks like this, listing the one IDP before the other in the dictionary of `allowed_idps`. I hoped we can could make this dictionary ordering be preserved all the way, and let it dictate in what order we provide CILogon with the `selected_idp` query parameter.

```yaml
  hub:
    config:
      JupyterHub:
        authenticator_class: cilogon
      CILogonOAuthenticator:
        oauth_callback_url: https://temple.2i2c.cloud/hub/oauth_callback
        allowed_idps:
          https://fim.temple.edu/idp/shibboleth:
            username_derivation:
              username_claim: "eppn"
            allow_all: true
          http://google.com/accounts/o8/id:
            username_derivation:
              username_claim: "email"
            allowed_domains:
              - "2i2c.org"
```

It turns out though, that z2jh will before this reaches JupyterHub and OAuthenticator have messed up the ordering of dictionary keys. This may be because of `helm`, or it may be because z2jh.

```yaml
    CILogonOAuthenticator:
      allowed_idps:
        http://google.com/accounts/o8/id:
          allowed_domains:
          - 2i2c.org
          username_derivation:
            username_claim: email
        https://fim.temple.edu/idp/shibboleth:
          allow_all: true
          username_derivation:
            username_claim: eppn
```

Looking at another example, it seems that the order of lists are preserved though. So, we could perhaps allow `allowed_idps` to also be provided as a list? If so, it could make it all the way in order to be transformed into an ordered dict for use by CILogonOAuthenticator.

Pull request:
[CILogon] add config to specify default idp under allowed_idps
- Closes #690 that includes motivation etc.

Patch:
diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index 8317e77..10fba6d 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -15,6 +15,24 @@ from .oauth2 import OAuthenticator, OAuthLoginHandler
 yaml = YAML(typ="safe", pure=True)
 
 
+def _get_select_idp_param(allowed_idps):
+    """
+    The "selected_idp" query parameter included when the user is redirected to
+    CILogon should be a comma separated string of idps to choose from, where the
+    first entry is pre-selected as the default choice. The ordering of the
+    remaining idps has no meaning.
+    """
+    # pick the first idp marked as default, or fallback to the first idp
+    default_keys = [k for k, v in allowed_idps.items() if v.get("default")]
+    default_key = next(iter(default_keys), next(iter(allowed_idps)))
+
+    # put the default idp first followed by the other idps
+    other_keys = [k for k, _ in allowed_idps.items() if k != default_key]
+    selected_idp = ",".join([default_key] + other_keys)
+
+    return selected_idp
+
+
 class CILogonLoginHandler(OAuthLoginHandler):
     """See https://www.cilogon.org/oidc for general information."""
 
@@ -29,10 +47,9 @@ class CILogonLoginHandler(OAuthLoginHandler):
         # include it, we then modify kwargs' extra_params dictionary
         extra_params = kwargs.setdefault('extra_params', {})
 
-        # selected_idp should be a comma separated string
-        allowed_idps = ",".join(self.authenticator.allowed_idps.keys())
-        extra_params["selected_idp"] = allowed_idps
-
+        extra_params["selected_idp"] = _get_select_idp_param(
+            self.authenticator.allowed_idps
+        )
         if self.authenticator.skin:
             extra_params["skin"] = self.authenticator.skin
 
@@ -124,6 +141,7 @@ class CILogonOAuthenticator(OAuthenticator):
                         "domain": "utoronto.ca",
                     },
                     "allow_all": True,
+                    "default": True,
                 },
                 "http://google.com/accounts/o8/id": {
                     "username_derivation": {
@@ -150,6 +168,9 @@ class CILogonOAuthenticator(OAuthenticator):
         This is a description of the configuration you can pass to
         `allowed_idps`.
 
+        * `default`: bool (optional)
+            Determines the identity provider to be pre-selected in a list for
+            users arriving to CILogons login screen.
         * `username_derivation`: string (required)
             * `username_claim`: string (required)
                 The claim in the `userinfo` response from which to define the


Test patch:
diff --git a/oauthenticator/tests/test_cilogon.py b/oauthenticator/tests/test_cilogon.py
index c638a0e..673835a 100644
--- a/oauthenticator/tests/test_cilogon.py
+++ b/oauthenticator/tests/test_cilogon.py
@@ -6,7 +6,7 @@ from pytest import fixture, mark, raises
 from traitlets.config import Config
 from traitlets.traitlets import TraitError
 
-from ..cilogon import CILogonOAuthenticator
+from ..cilogon import CILogonOAuthenticator, _get_select_idp_param
 from .mocks import setup_oauth_mock
 
 
@@ -675,3 +675,44 @@ async def test_allowed_idps_username_derivation_actions(cilogon_client):
     auth_model = await authenticator.get_authenticated_user(handler, None)
     print(json.dumps(auth_model, sort_keys=True, indent=4))
     assert auth_model['name'] == 'jtkirk'
+
+
+@mark.parametrize(
+    "test_variation_id,allowed_idps,expected_return_value",
+    [
+        (
+            "default-specified",
+            {
+                'https://example4.org': {},
+                'https://example3.org': {'default': False},
+                'https://example2.org': {'default': True},
+                'https://example1.org': {},
+            },
+            "https://example2.org,https://example4.org,https://example3.org,https://example1.org",
+        ),
+        (
+            "no-truthy-default-specified",
+            {
+                'https://example4.org': {},
+                'https://example3.org': {'default': False},
+                'https://example2.org': {},
+                'https://example1.org': {},
+            },
+            "https://example4.org,https://example3.org,https://example2.org,https://example1.org",
+        ),
+        (
+            "no-default-specified-pick-first-entry",
+            {
+                'https://example4.org': {},
+                'https://example3.org': {},
+                'https://example2.org': {},
+                'https://example1.org': {},
+            },
+            "https://example4.org,https://example3.org,https://example2.org,https://example1.org",
+        ),
+    ],
+)
+async def test__get_selected_idp_param(
+    test_variation_id, allowed_idps, expected_return_value
+):
+    assert _get_select_idp_param(allowed_idps) == expected_return_value