Repository: jupyterhub/oauthenticator

Issue:
[CILogon] Rename `allowed_idps` to `idps` in a non-breaking way?
I think the name `allowed_idps` causes some confusion in oauthenticator v16, and that it should be non-breakingly renamed to just `idps` with a deprecation warning for users still using the `allowed_idps` name.

With a name like `idps`, there is no hint about users from a specific ipd would be allowed or simiarly. Configuring idps is about describing what users we can authenticate/recognize, while `allowed` refers to what users we authorize/allow. Since configuring an idp to be used to authenticate a user doesn't go hand in hand with authorizing a user, I think it should be renamed for clarity.

Together with the proposal in #682, the config for CILogon that is associated with authorizing users then become the following:

- `OAuthenticator.allow_all`
- `OAuthenticator.allowed_users`
- `OAuthenticator.admin_users`
- `OAuthenticator.allow_existing_users`
- `CILogonOAuthenticator.idps[<some idp>].allowed_domains`
- `CILogonOAuthenticator.idps[<some idp>].allow_all`

## Proposal

Rename `allowed_idps` to `idps`, making `allowed_idps` still work as an alias for `idps`, but come with a deprecation warning.

Discussion:

Patch:
diff --git a/docs/source/tutorials/provider-specific-setup/providers/cilogon.md b/docs/source/tutorials/provider-specific-setup/providers/cilogon.md
index 0c76e3d..1b1bf0b 100644
--- a/docs/source/tutorials/provider-specific-setup/providers/cilogon.md
+++ b/docs/source/tutorials/provider-specific-setup/providers/cilogon.md
@@ -21,4 +21,4 @@ c.OAuthenticator.client_secret = "[your oauth2 application secret]"
 CILogonOAuthenticator expands OAuthenticator with the following required config,
 read more about it in the configuration reference:
 
-- {attr}`.CILogonOAuthenticator.allowed_idps`
+- {attr}`.CILogonOAuthenticator.idps`
diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index bf08f14..0075ecc 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -17,7 +17,7 @@ from .oauth2 import OAuthenticator, OAuthLoginHandler
 yaml = YAML(typ="safe", pure=True)
 
 
-def _get_select_idp_param(allowed_idps):
+def _get_select_idp_param(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
@@ -25,11 +25,11 @@ def _get_select_idp_param(allowed_idps):
     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)))
+    default_keys = [k for k, v in idps.items() if v.get("default")]
+    default_key = next(iter(default_keys), next(iter(idps)))
 
     # put the default idp first followed by the other idps
-    other_keys = [k for k, _ in allowed_idps.items() if k != default_key]
+    other_keys = [k for k, _ in idps.items() if k != default_key]
     selected_idp = ",".join([default_key] + other_keys)
 
     return selected_idp
@@ -41,7 +41,7 @@ class CILogonLoginHandler(OAuthLoginHandler):
     def authorize_redirect(self, *args, **kwargs):
         """
         Optionally add "skin" to redirect params, and always add "selected_idp"
-        (aka. "idphint") based on allowed_idps config.
+        (aka. "idphint") based on idps config.
 
         Related documentation at https://www.cilogon.org/oidc#h.p_IWGvXH0okDI_.
         """
@@ -49,9 +49,7 @@ class CILogonLoginHandler(OAuthLoginHandler):
         # include it, we then modify kwargs' extra_params dictionary
         extra_params = kwargs.setdefault('extra_params', {})
 
-        extra_params["selected_idp"] = _get_select_idp_param(
-            self.authenticator.allowed_idps
-        )
+        extra_params["selected_idp"] = _get_select_idp_param(self.authenticator.idps)
         if self.authenticator.skin:
             extra_params["skin"] = self.authenticator.skin
 
@@ -123,7 +121,7 @@ class CILogonOAuthenticator(OAuthenticator):
 
         return scopes
 
-    allowed_idps = Dict(
+    idps = Dict(
         config=True,
         help="""
         A dictionary of the only entity IDs that will be allowed to be used as
@@ -135,7 +133,7 @@ class CILogonOAuthenticator(OAuthenticator):
 
         For example::
 
-            c.CILogonOAuthenticator.allowed_idps = {
+            c.CILogonOAuthenticator.idps = {
                 "https://idpz.utorauth.utoronto.ca/shibboleth": {
                     "username_derivation": {
                         "username_claim": "email",
@@ -168,7 +166,7 @@ class CILogonOAuthenticator(OAuthenticator):
             c.Authenticator.allowed_users = ["github-user2"]
 
         This is a description of the configuration you can pass to
-        `allowed_idps`.
+        `idps`.
 
         * `default`: bool (optional)
             Determines the identity provider to be pre-selected in a list for
@@ -221,12 +219,12 @@ class CILogonOAuthenticator(OAuthenticator):
         """,
     )
 
-    @validate("allowed_idps")
-    def _validate_allowed_idps(self, proposal):
+    @validate("idps")
+    def _validate_idps(self, proposal):
         idps = proposal.value
 
         if not idps:
-            raise ValueError("One or more allowed_idps must be configured")
+            raise ValueError("One or more idps must be configured")
 
         for entity_id, idp_config in idps.items():
             # Validate `idp_config` config using the schema
@@ -237,7 +235,7 @@ class CILogonOAuthenticator(OAuthenticator):
                 # Raises useful exception if validation fails
                 jsonschema.validate(idp_config, schema)
 
-            # Make sure allowed_idps contains EntityIDs and not domain names.
+            # Make sure idps contains EntityIDs and not domain names.
             accepted_entity_id_scheme = ["urn", "https", "http"]
             entity_id_scheme = urlparse(entity_id).scheme
             if entity_id_scheme not in accepted_entity_id_scheme:
@@ -246,7 +244,7 @@ class CILogonOAuthenticator(OAuthenticator):
                     f"Trying to allow an auth provider: {entity_id}, that doesn't look like a valid CILogon EntityID.",
                 )
                 raise ValueError(
-                    "The keys of `allowed_idps` **must** be CILogon permitted EntityIDs. "
+                    "The keys of `idps` **must** be CILogon permitted EntityIDs. "
                     "See https://cilogon.org/idplist for the list of EntityIDs of each IDP."
                 )
 
@@ -269,12 +267,13 @@ class CILogonOAuthenticator(OAuthenticator):
 
     # _deprecated_oauth_aliases is used by deprecation logic in OAuthenticator
     _deprecated_oauth_aliases = {
-        "idp_whitelist": ("allowed_idps", "0.12.0", False),
+        "idp_whitelist": ("idps", "0.12.0", False),
         "idp": ("shown_idps", "15.0.0", False),
-        "strip_idp_domain": ("allowed_idps", "15.0.0", False),
-        "shown_idps": ("allowed_idps", "16.0.0", False),
-        "additional_username_claims": ("allowed_idps", "16.0.0", False),
-        "username_claim": ("allowed_idps", "16.0.0", False),
+        "strip_idp_domain": ("idps", "15.0.0", False),
+        "shown_idps": ("idps", "16.0.0", False),
+        "additional_username_claims": ("idps", "16.0.0", False),
+        "username_claim": ("idps", "16.0.0", False),
+        "allowed_idps": ("idps", "17.4.0", True),
         **OAuthenticator._deprecated_oauth_aliases,
     }
     idp_whitelist = List(
@@ -282,7 +281,7 @@ class CILogonOAuthenticator(OAuthenticator):
         help="""
         .. versionremoved:: 0.12
 
-           Use :attr:`allowed_idps`.
+           Use :attr:`idps`.
         """,
     )
     idp = Unicode(
@@ -290,7 +289,7 @@ class CILogonOAuthenticator(OAuthenticator):
         help="""
         .. versionremoved:: 15.0
 
-           Use :attr:`allowed_idps`.
+           Use :attr:`idps`.
         """,
     )
     strip_idp_domain = Bool(
@@ -298,7 +297,7 @@ class CILogonOAuthenticator(OAuthenticator):
         help="""
         .. versionremoved:: 15.0
 
-           Use :attr:`allowed_idps`.
+           Use :attr:`idps`.
         """,
     )
     shown_idps = List(
@@ -306,7 +305,7 @@ class CILogonOAuthenticator(OAuthenticator):
         help="""
         .. versionremoved:: 16.0
 
-           Use :attr:`allowed_idps`.
+           Use :attr:`idps`.
         """,
     )
     additional_username_claims = List(
@@ -314,7 +313,7 @@ class CILogonOAuthenticator(OAuthenticator):
         help="""
         .. versionremoved:: 16.0
 
-           Use :attr:`allowed_idps`.
+           Use :attr:`idps`.
         """,
     )
     username_claim = Unicode(
@@ -322,7 +321,15 @@ class CILogonOAuthenticator(OAuthenticator):
         help="""
         .. versionremoved:: 16.0
 
-           Use :attr:`allowed_idps`.
+           Use :attr:`idps`.
+        """,
+    )
+    allowed_idps = Dict(
+        config=True,
+        help="""
+        .. deprecated:: 17.4
+
+           Use :attr:`idps`.
         """,
     )
 
@@ -330,22 +337,22 @@ class CILogonOAuthenticator(OAuthenticator):
         """
         Overrides OAuthenticator.user_info_to_username that relies on
         username_claim to instead consider idp specific config in under
-        allowed_idps[user_info["idp"]]["username_derivation"].
+        idps[user_info["idp"]]["username_derivation"].
 
-        Returns a username based on user_info and configuration in allowed_idps
+        Returns a username based on user_info and configuration in idps
         under the associated idp's username_derivation config.
         """
         # NOTE: The first time we have received user_info is when
         #       user_info_to_username is called by OAuthenticator.authenticate,
         #       so we make a check here that the "idp" claim is received and
-        #       that we allowed_idps is configured to handle that idp.
+        #       that we idps is configured to handle that idp.
         #
         user_idp = user_info.get("idp")
         if not user_idp:
             message = "'idp' claim was not part of the response to the userdata_url"
             self.log.error(message)
             raise web.HTTPError(500, message)
-        if not self.allowed_idps.get(user_idp):
+        if not self.idps.get(user_idp):
             message = f"Login with identity provider {user_idp} is not pre-configured"
             self.log.error(message)
             raise web.HTTPError(403, message)
@@ -361,7 +368,7 @@ class CILogonOAuthenticator(OAuthenticator):
         specified under "username_derivation" for the associated idp.
         """
         user_idp = user_info["idp"]
-        username_derivation = self.allowed_idps[user_idp]["username_derivation"]
+        username_derivation = self.idps[user_idp]["username_derivation"]
         username_claim = username_derivation["username_claim"]
 
         username = user_info.get(username_claim)
@@ -378,7 +385,7 @@ class CILogonOAuthenticator(OAuthenticator):
         specified under "username_derivation" for the associated idp.
         """
         user_idp = user_info["idp"]
-        username_derivation = self.allowed_idps[user_idp]["username_derivation"]
+        username_derivation = self.idps[user_idp]["username_derivation"]
 
         # Optionally execute action "strip_idp_domain" or "prefix"
         action = username_derivation.get("action")
@@ -396,7 +403,7 @@ class CILogonOAuthenticator(OAuthenticator):
         """
         Overrides the OAuthenticator.check_allowed to also allow users based on
         idp specific config `allow_all` and `allowed_domains` as configured
-        under `allowed_idps`.
+        under `idps`.
         """
         if await super().check_allowed(username, auth_model):
             return True
@@ -404,15 +411,13 @@ class CILogonOAuthenticator(OAuthenticator):
         user_info = auth_model["auth_state"][self.user_auth_state_key]
         user_idp = user_info["idp"]
 
-        idp_allow_all = self.allowed_idps[user_idp].get("allow_all")
+        idp_allow_all = self.idps[user_idp].get("allow_all")
         if idp_allow_all:
             return True
 
-        idp_allowed_domains = self.allowed_idps[user_idp].get("allowed_domains")
+        idp_allowed_domains = self.idps[user_idp].get("allowed_domains")
         if idp_allowed_domains:
-            idp_allowed_domains_claim = self.allowed_idps[user_idp].get(
-                "allowed_domains_claim"
-            )
+            idp_allowed_domains_claim = self.idps[user_idp].get("allowed_domains_claim")
             if idp_allowed_domains_claim:
                 raw_user_domain = user_info.get(idp_allowed_domains_claim)
                 if not raw_user_domain:
diff --git a/oauthenticator/schemas/cilogon-schema.yaml b/oauthenticator/schemas/cilogon-schema.yaml
index 2f8436f..4a9b55f 100644
--- a/oauthenticator/schemas/cilogon-schema.yaml
+++ b/oauthenticator/schemas/cilogon-schema.yaml
@@ -1,5 +1,5 @@
 # This JSONSchema is used to validate the values in the
-# CILogonOAuthenticator.allowed_idps dictionary.
+# CILogonOAuthenticator.idps dictionary.
 #
 $schema: http://json-schema.org/draft-07/schema#
 type: object


Test patch:
diff --git a/oauthenticator/tests/test_cilogon.py b/oauthenticator/tests/test_cilogon.py
index 2b830a4..94c59ba 100644
--- a/oauthenticator/tests/test_cilogon.py
+++ b/oauthenticator/tests/test_cilogon.py
@@ -115,7 +115,7 @@ async def test_cilogon(
     print(f"Running test variation id {test_variation_id}")
     c = Config()
     c.CILogonOAuthenticator = Config(class_config)
-    c.CILogonOAuthenticator.allowed_idps = {
+    c.CILogonOAuthenticator.idps = {
         "https://some-idp.com/login/oauth/authorize": {
             "username_derivation": {
                 "username_claim": "name",
@@ -436,7 +436,7 @@ async def test_cilogon(
         ),
     ],
 )
-async def test_cilogon_allowed_idps(
+async def test_cilogon_idps(
     cilogon_client,
     test_variation_id,
     idp_config,
@@ -449,7 +449,7 @@ async def test_cilogon_allowed_idps(
     c = Config()
     c.CILogonOAuthenticator = Config(class_config)
     test_idp = "https://some-idp.com/login/oauth/authorize"
-    c.CILogonOAuthenticator.allowed_idps = {
+    c.CILogonOAuthenticator.idps = {
         test_idp: idp_config,
     }
     authenticator = CILogonOAuthenticator(config=c)
@@ -484,7 +484,7 @@ async def test_cilogon_allowed_idps(
             {"idp_whitelist": ["dummy"]},
             {},
             logging.ERROR,
-            "CILogonOAuthenticator.idp_whitelist is deprecated in CILogonOAuthenticator 0.12.0, use CILogonOAuthenticator.allowed_idps instead",
+            "CILogonOAuthenticator.idp_whitelist is deprecated in CILogonOAuthenticator 0.12.0, use CILogonOAuthenticator.idps instead",
         ),
         (
             "idp",
@@ -498,28 +498,48 @@ async def test_cilogon_allowed_idps(
             {"strip_idp_domain": True},
             {},
             logging.ERROR,
-            "CILogonOAuthenticator.strip_idp_domain is deprecated in CILogonOAuthenticator 15.0.0, use CILogonOAuthenticator.allowed_idps instead",
+            "CILogonOAuthenticator.strip_idp_domain is deprecated in CILogonOAuthenticator 15.0.0, use CILogonOAuthenticator.idps instead",
         ),
         (
             "shown_idps",
             {"shown_idps": ["dummy"]},
             {},
             logging.ERROR,
-            "CILogonOAuthenticator.shown_idps is deprecated in CILogonOAuthenticator 16.0.0, use CILogonOAuthenticator.allowed_idps instead",
+            "CILogonOAuthenticator.shown_idps is deprecated in CILogonOAuthenticator 16.0.0, use CILogonOAuthenticator.idps instead",
         ),
         (
             "username_claim",
             {"username_claim": "dummy"},
             {},
             logging.ERROR,
-            "CILogonOAuthenticator.username_claim is deprecated in CILogonOAuthenticator 16.0.0, use CILogonOAuthenticator.allowed_idps instead",
+            "CILogonOAuthenticator.username_claim is deprecated in CILogonOAuthenticator 16.0.0, use CILogonOAuthenticator.idps instead",
         ),
         (
             "additional_username_claims",
             {"additional_username_claims": ["dummy"]},
             {},
             logging.ERROR,
-            "CILogonOAuthenticator.additional_username_claims is deprecated in CILogonOAuthenticator 16.0.0, use CILogonOAuthenticator.allowed_idps instead",
+            "CILogonOAuthenticator.additional_username_claims is deprecated in CILogonOAuthenticator 16.0.0, use CILogonOAuthenticator.idps instead",
+        ),
+        (
+            "allowed_idps",
+            {
+                "allowed_idps": {
+                    "https://github.com/login/oauth/authorize": {
+                        "username_derivation": {"username_claim": "email"}
+                    }
+                }
+            },
+            {
+                "idps": {
+                    "https://github.com/login/oauth/authorize": {
+                        'allowed_domains': [],
+                        "username_derivation": {"username_claim": "email"},
+                    }
+                }
+            },
+            logging.WARNING,
+            "CILogonOAuthenticator.allowed_idps is deprecated in CILogonOAuthenticator 17.4.0, use CILogonOAuthenticator.idps instead",
         ),
     ],
 )
@@ -555,21 +575,21 @@ async def test_deprecated_config(
     assert expected_log_tuple in captured_log_tuples
 
 
-async def test_config_allowed_idps_wrong_type(caplog):
+async def test_config_idps_wrong_type(caplog):
     """
     Test alllowed_idps is a dict
     """
     c = Config()
-    c.CILogonOAuthenticator.allowed_idps = ['pink']
+    c.CILogonOAuthenticator.idps = ['pink']
 
     with raises(TraitError):
         CILogonOAuthenticator(config=c)
 
 
-async def test_config_allowed_idps_required_username_derivation(caplog):
-    # Test username_derivation is a required field of allowed_idps
+async def test_config_idps_required_username_derivation(caplog):
+    # Test username_derivation is a required field of idps
     c = Config()
-    c.CILogonOAuthenticator.allowed_idps = {
+    c.CILogonOAuthenticator.idps = {
         'https://github.com/login/oauth/authorize': {},
     }
 
@@ -577,13 +597,13 @@ async def test_config_allowed_idps_required_username_derivation(caplog):
         CILogonOAuthenticator(config=c)
 
 
-async def test_config_allowed_idps_invalid_entity_id(caplog):
+async def test_config_idps_invalid_entity_id(caplog):
     """
-    Test allowed_idps keys cannot be domains, but only valid CILogon entity ids,
+    Test idps keys cannot be domains, but only valid CILogon entity ids,
     i.e. only fully formed URLs
     """
     c = Config()
-    c.CILogonOAuthenticator.allowed_idps = {
+    c.CILogonOAuthenticator.idps = {
         'uni.edu': {
             'username_derivation': {
                 'username_claim': 'email',
@@ -606,18 +626,18 @@ async def test_config_allowed_idps_invalid_entity_id(caplog):
     assert expected_deprecation_error in log_msgs
 
 
-async def test_config_allowed_idps_invalid_type(caplog):
+async def test_config_idps_invalid_type(caplog):
     c = Config()
-    c.CILogonOAuthenticator.allowed_idps = {
+    c.CILogonOAuthenticator.idps = {
         'https://github.com/login/oauth/authorize': 'should-be-a-dict'
     }
     with raises(ValidationError, match="'should-be-a-dict' is not of type 'object'"):
         CILogonOAuthenticator(config=c)
 
 
-async def test_config_allowed_idps_unrecognized_options(caplog):
+async def test_config_idps_unrecognized_options(caplog):
     c = Config()
-    c.CILogonOAuthenticator.allowed_idps = {
+    c.CILogonOAuthenticator.idps = {
         'https://github.com/login/oauth/authorize': {
             'username_derivation': {'a': 1, 'b': 2}
         }
@@ -626,9 +646,9 @@ async def test_config_allowed_idps_unrecognized_options(caplog):
         CILogonOAuthenticator(config=c)
 
 
-async def test_config_allowed_idps_domain_required(caplog):
+async def test_config_idps_domain_required(caplog):
     c = Config()
-    c.CILogonOAuthenticator.allowed_idps = {
+    c.CILogonOAuthenticator.idps = {
         'https://github.com/login/oauth/authorize': {
             'username_derivation': {
                 'username_claim': 'email',
@@ -640,9 +660,9 @@ async def test_config_allowed_idps_domain_required(caplog):
         CILogonOAuthenticator(config=c)
 
 
-async def test_config_allowed_idps_prefix_required(caplog):
+async def test_config_idps_prefix_required(caplog):
     c = Config()
-    c.CILogonOAuthenticator.allowed_idps = {
+    c.CILogonOAuthenticator.idps = {
         'https://github.com/login/oauth/authorize': {
             'username_derivation': {
                 'username_claim': 'email',
@@ -659,7 +679,7 @@ async def test_config_scopes_validation():
     Test that required scopes are appended if not configured.
     """
     c = Config()
-    c.CILogonOAuthenticator.allowed_idps = {
+    c.CILogonOAuthenticator.idps = {
         'https://some-idp.com/login/oauth/authorize': {
             'username_derivation': {
                 'username_claim': 'email',
@@ -675,14 +695,14 @@ async def test_config_scopes_validation():
     assert authenticator.scope == expected_scopes
 
 
-async def test_allowed_idps_username_derivation_actions(cilogon_client):
+async def test_idps_username_derivation_actions(cilogon_client):
     """
-    Tests all `allowed_idps[].username_derivation.action` config choices:
+    Tests all `idps[].username_derivation.action` config choices:
     `strip_idp_domain`, `prefix`, and no action specified.
     """
     c = Config()
     c.CILogonOAuthenticator.allow_all = True
-    c.CILogonOAuthenticator.allowed_idps = {
+    c.CILogonOAuthenticator.idps = {
         'https://strip-idp-domain.example.com/login/oauth/authorize': {
             'default': True,
             'username_derivation': {
@@ -754,7 +774,7 @@ async def test_allowed_idps_username_derivation_actions(cilogon_client):
 
 
 @mark.parametrize(
-    "test_variation_id,allowed_idps,expected_return_value",
+    "test_variation_id,idps,expected_return_value",
     [
         (
             "default-specified",
@@ -788,7 +808,5 @@ async def test_allowed_idps_username_derivation_actions(cilogon_client):
         ),
     ],
 )
-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
+async def test__get_selected_idp_param(test_variation_id, idps, expected_return_value):
+    assert _get_select_idp_param(idps) == expected_return_value