Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CILogon] Rename allowed_idps to idps (deprecation, not removal) #685

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

[CILogon] Rename allowed_idps to idps (deprecation, not removal) #685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6576b09
cilogon: rename allowed_idps to idps, deprecating old name
consideRatio Sep 25, 2023
a65d7aa
cilogon: test with new name, old name confirmed to work
consideRatio Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/source/tutorials/provider-specific-setup/providers/cilogon.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
71 changes: 40 additions & 31 deletions oauthenticator/cilogon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_.
"""
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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",
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -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."
)

Expand All @@ -223,83 +223,92 @@ 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(
config=True,
help="""
.. versionremoved:: 0.12

Use :attr:`allowed_idps`.
Use :attr:`idps`.
""",
)
idp = Unicode(
config=True,
help="""
.. versionremoved:: 15.0

Use :attr:`allowed_idps`.
Use :attr:`idps`.
""",
)
strip_idp_domain = Bool(
config=True,
help="""
.. versionremoved:: 15.0

Use :attr:`allowed_idps`.
Use :attr:`idps`.
""",
)
shown_idps = List(
config=True,
help="""
.. versionremoved:: 16.0

Use :attr:`allowed_idps`.
Use :attr:`idps`.
""",
)
additional_username_claims = List(
config=True,
help="""
.. versionremoved:: 16.0

Use :attr:`allowed_idps`.
Use :attr:`idps`.
""",
)
username_claim = Unicode(
config=True,
help="""
.. versionremoved:: 16.0

Use :attr:`allowed_idps`.
Use :attr:`idps`.
""",
)
allowed_idps = Dict(
config=True,
help="""
.. versiondeprecated:: 16.1

Use :attr:`idps`.
""",
)

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)
Expand All @@ -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)
Expand All @@ -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")
Expand All @@ -350,19 +359,19 @@ 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

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()
Expand Down
2 changes: 1 addition & 1 deletion oauthenticator/schemas/cilogon-schema.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Loading