From 6576b09c0fed5d6531c71872a91f1acca4c3eedb Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Mon, 25 Sep 2023 12:16:19 +0200 Subject: [PATCH] cilogon: rename allowed_idps to idps, deprecating old name --- .../providers/cilogon.md | 2 +- oauthenticator/cilogon.py | 71 +++++++++++-------- oauthenticator/schemas/cilogon-schema.yaml | 2 +- oauthenticator/tests/test_cilogon.py | 45 ++++++++---- 4 files changed, 74 insertions(+), 46 deletions(-) diff --git a/docs/source/tutorials/provider-specific-setup/providers/cilogon.md b/docs/source/tutorials/provider-specific-setup/providers/cilogon.md index 0c76e3df..1b1bf0be 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 8317e776..0e3ede47 100644 --- a/oauthenticator/cilogon.py +++ b/oauthenticator/cilogon.py @@ -21,7 +21,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_. """ @@ -30,8 +30,8 @@ def authorize_redirect(self, *args, **kwargs): 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 + idps = ",".join(self.authenticator.idps.keys()) + extra_params["selected_idp"] = idps if self.authenticator.skin: extra_params["skin"] = self.authenticator.skin @@ -104,7 +104,7 @@ def _validate_scope(self, proposal): 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 @@ -116,7 +116,7 @@ def _validate_scope(self, proposal): For example:: - c.CILogonOAuthenticator.allowed_idps = { + c.CILogonOAuthenticator.idps = { "https://idpz.utorauth.utoronto.ca/shibboleth": { "username_derivation": { "username_claim": "email", @@ -148,7 +148,7 @@ def _validate_scope(self, proposal): c.Authenticator.allowed_users = ["github-user2"] This is a description of the configuration you can pass to - `allowed_idps`. + `idps`. * `username_derivation`: string (required) * `username_claim`: string (required) @@ -180,12 +180,12 @@ def _validate_scope(self, proposal): """, ) - @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 @@ -196,7 +196,7 @@ def _validate_allowed_idps(self, proposal): # 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: @@ -205,7 +205,7 @@ def _validate_allowed_idps(self, proposal): 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." ) @@ -223,12 +223,13 @@ def _validate_allowed_idps(self, proposal): # _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", "16.1.0", True), **OAuthenticator._deprecated_oauth_aliases, } idp_whitelist = List( @@ -236,7 +237,7 @@ def _validate_allowed_idps(self, proposal): help=""" .. versionremoved:: 0.12 - Use :attr:`allowed_idps`. + Use :attr:`idps`. """, ) idp = Unicode( @@ -244,7 +245,7 @@ def _validate_allowed_idps(self, proposal): help=""" .. versionremoved:: 15.0 - Use :attr:`allowed_idps`. + Use :attr:`idps`. """, ) strip_idp_domain = Bool( @@ -252,7 +253,7 @@ def _validate_allowed_idps(self, proposal): help=""" .. versionremoved:: 15.0 - Use :attr:`allowed_idps`. + Use :attr:`idps`. """, ) shown_idps = List( @@ -260,7 +261,7 @@ def _validate_allowed_idps(self, proposal): help=""" .. versionremoved:: 16.0 - Use :attr:`allowed_idps`. + Use :attr:`idps`. """, ) additional_username_claims = List( @@ -268,7 +269,7 @@ def _validate_allowed_idps(self, proposal): help=""" .. versionremoved:: 16.0 - Use :attr:`allowed_idps`. + Use :attr:`idps`. """, ) username_claim = Unicode( @@ -276,7 +277,15 @@ def _validate_allowed_idps(self, proposal): help=""" .. versionremoved:: 16.0 - Use :attr:`allowed_idps`. + Use :attr:`idps`. + """, + ) + allowed_idps = Dict( + config=True, + help=""" + .. versiondeprecated:: 16.1 + + Use :attr:`idps`. """, ) @@ -284,22 +293,22 @@ def user_info_to_username(self, user_info): """ 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) @@ -315,7 +324,7 @@ def _user_info_to_unprocessed_username(self, user_info): 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) @@ -332,7 +341,7 @@ def _get_processed_username(self, username, user_info): 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") @@ -350,7 +359,7 @@ async def check_allowed(self, username, auth_model): """ 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 @@ -358,11 +367,11 @@ async def check_allowed(self, username, auth_model): 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: unprocessed_username = self._user_info_to_unprocessed_username(user_info) user_domain = unprocessed_username.split("@", 1)[1].lower() diff --git a/oauthenticator/schemas/cilogon-schema.yaml b/oauthenticator/schemas/cilogon-schema.yaml index fb791599..e5f36eb1 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 diff --git a/oauthenticator/tests/test_cilogon.py b/oauthenticator/tests/test_cilogon.py index c638a0e0..56d6ed10 100644 --- a/oauthenticator/tests/test_cilogon.py +++ b/oauthenticator/tests/test_cilogon.py @@ -364,7 +364,7 @@ async def test_cilogon( ), ], ) -async def test_cilogon_allowed_idps( +async def test_cilogon_idps( cilogon_client, test_variation_id, idp_config, @@ -409,7 +409,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", @@ -423,28 +423,47 @@ 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": { + "username_derivation": {"username_claim": "email"} + } + } + }, + logging.WARNING, + "CILogonOAuthenticator.allowed_idps is deprecated in CILogonOAuthenticator 16.1.0, use CILogonOAuthenticator.idps instead", ), ], ) @@ -480,7 +499,7 @@ 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 """ @@ -491,7 +510,7 @@ async def test_config_allowed_idps_wrong_type(caplog): CILogonOAuthenticator(config=c) -async def test_config_allowed_idps_required_username_derivation(caplog): +async def test_config_idps_required_username_derivation(caplog): # Test username_derivation is a required field of allowed_idps c = Config() c.CILogonOAuthenticator.allowed_idps = { @@ -502,7 +521,7 @@ 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, i.e. only fully formed URLs @@ -531,7 +550,7 @@ 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 = { 'https://github.com/login/oauth/authorize': 'should-be-a-dict' @@ -540,7 +559,7 @@ async def test_config_allowed_idps_invalid_type(caplog): 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 = { 'https://github.com/login/oauth/authorize': { @@ -551,7 +570,7 @@ 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 = { 'https://github.com/login/oauth/authorize': { @@ -565,7 +584,7 @@ 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 = { 'https://github.com/login/oauth/authorize': {