diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md index d4e7da3b..610d8d1a 100644 --- a/docs/source/reference/changelog.md +++ b/docs/source/reference/changelog.md @@ -23,6 +23,8 @@ command line for details. lists as as they are converted to sets automatically, but anyone reading and adding entries must now use set logic and not list logic. - [Google] Authentication state's `google_groups` is now a set, not a list. +- [CILogon] `allowed_idps` is now required config, and `shown_idps`, + `username_claim`, `additional_username_claims` must no longer be configured. (changelog:version-15)= diff --git a/oauthenticator/auth0.py b/oauthenticator/auth0.py index 909551e6..203df5dd 100644 --- a/oauthenticator/auth0.py +++ b/oauthenticator/auth0.py @@ -54,13 +54,15 @@ class Auth0OAuthenticator(OAuthenticator): def _username_claim_default(self): return "email" - auth0_subdomain = Unicode(config=True) - auth0_domain = Unicode(config=True) + auth0_domain = Unicode( + config=True, + help=""" + The domain for your Auth0 account. - @default("auth0_subdomain") - def _auth0_subdomain_default(self): - # This is allowed to be empty unless auth0_domain is not supplied either - return os.getenv("AUTH0_SUBDOMAIN", "") + Used to determine the default values for `logout_redirect_url`, + `authorize_url`, `token_url`, and `userdata_url`. + """, + ) @default("auth0_domain") def _auth0_domain_default(self): @@ -70,12 +72,23 @@ def _auth0_domain_default(self): if self.auth0_subdomain: return f"{self.auth0_subdomain}.auth0.com" raise ValueError( - "Please specify $AUTH0_DOMAIN env, $AUTH0_SUBDOMAIN env, " - "{part}.auth0_domain config, or {part}.auth0_subdomain config".format( - part=self.__class__.__name__ - ) + "Configuring either auth0_domain or auth0_subdomain is required" ) + auth0_subdomain = Unicode( + config=True, + help=""" + A shorthand for configuring `auth0_domain`, if configured to + "something", it is the same as configuring `auth0_domain` to + "something.auth0.com". + """, + ) + + @default("auth0_subdomain") + def _auth0_subdomain_default(self): + # This is allowed to be empty unless auth0_domain is not supplied either + return os.getenv("AUTH0_SUBDOMAIN", "") + username_key = Unicode( config=True, help="Deprecated, use `Auth0OAuthenticator.username_claim`", diff --git a/oauthenticator/bitbucket.py b/oauthenticator/bitbucket.py index 1a22f147..ecd75248 100644 --- a/oauthenticator/bitbucket.py +++ b/oauthenticator/bitbucket.py @@ -71,33 +71,19 @@ async def update_auth_model(self, auth_model): async def check_allowed(self, username, auth_model): """ - Returns True for users allowed to be authorized. - - Overrides the OAuthenticator.check_allowed implementation to allow users - either part of `allowed_users` or `allowed_teams`, and not just those - part of `allowed_users`. + Overrides the OAuthenticator.check_allowed to also allow users part of + `allowed_teams`. """ - # A workaround for JupyterHub<=4.0.1, described in - # https://github.com/jupyterhub/oauthenticator/issues/621 - if auth_model is None: - return True - - # allow admin users recognized via admin_users or update_auth_model - if auth_model["admin"]: + if await super().check_allowed(username, auth_model): return True - # if allowed_users or allowed_teams is configured, we deny users not - # part of either - if self.allowed_users or self.allowed_teams: + if self.allowed_teams: user_teams = auth_model["auth_state"]["user_teams"] - if username in self.allowed_users: - return True if any(user_teams & self.allowed_teams): return True - return False - # otherwise, authorize all users - return True + # users should be explicitly allowed via config, otherwise they aren't + return False class LocalBitbucketOAuthenticator(LocalAuthenticator, BitbucketOAuthenticator): diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py index b1aee5ea..39d942d8 100644 --- a/oauthenticator/cilogon.py +++ b/oauthenticator/cilogon.py @@ -30,13 +30,20 @@ class CILogonLoginHandler(OAuthLoginHandler): """See https://www.cilogon.org/oidc for general information.""" def authorize_redirect(self, *args, **kwargs): - """Add idp, skin to redirect params""" + """ + Optionally add "skin" to redirect params, and always add "selected_idp" + (aka. "idphint") based on allowed_idps config. + + Related documentation at https://www.cilogon.org/oidc#h.p_IWGvXH0okDI_. + """ + # kwargs is updated to include extra_params if it doesn't already + # include it, we then modify kwargs' extra_params dictionary extra_params = kwargs.setdefault('extra_params', {}) - if self.authenticator.shown_idps: - # selected_idp must be a string where idps are separated by commas, with no space between, otherwise it will get escaped - # example: https://accounts.google.com/o/oauth2/auth,https://github.com/login/oauth/authorize - idps = ",".join(self.authenticator.shown_idps) - extra_params["selected_idp"] = idps + + # selected_idp should be a comma separated string + allowed_idps = ",".join(self.authenticator.allowed_idps.keys()) + extra_params["selected_idp"] = allowed_idps + if self.authenticator.skin: extra_params["skin"] = self.authenticator.skin @@ -54,6 +61,9 @@ class CILogonOAuthenticator(OAuthenticator): "idp_whitelist": ("allowed_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), + "username_claim": ("allowed_idps", "16.0.0", False), + "additional_username_claims": ("allowed_idps", "16.0.0", False), **OAuthenticator._deprecated_oauth_aliases, } @@ -92,24 +102,29 @@ def _username_claim_default(self): Unicode(), default_value=['openid', 'email', 'org.cilogon.userinfo', 'profile'], config=True, - help="""The OAuth scopes to request. + help=""" + The OAuth scopes to request. - See cilogon_scope.md for details. - At least 'openid' is required. + See cilogon_scope.md for details. At least 'openid' is required. """, ) @validate('scope') def _validate_scope(self, proposal): - """Ensure `openid` is always requested and `org.cilogon.userinfo` - is requested when allowed_idps is specified. + """ + Ensure `openid` and `org.cilogon.userinfo` is requested. + + - The `idp` claim is required, and its documented to associate with + requesting the `org.cilogon.userinfo` scope. + + ref: https://www.cilogon.org/oidc#h.p_PEQXL8QUjsQm """ scopes = proposal.value if 'openid' not in proposal.value: scopes += ['openid'] - if self.allowed_idps and 'org.cilogon.userinfo' not in proposal.value: + if 'org.cilogon.userinfo' not in proposal.value: scopes += ['org.cilogon.userinfo'] return scopes @@ -122,49 +137,58 @@ def _validate_scope(self, proposal): allowed_idps = Dict( config=True, default_value={}, - help="""A dictionary of the only entity IDs that will be allowed to be used as login options. - See https://cilogon.org/idplist for the list of `EntityIDs` of each IdP. + help=""" + A dictionary of the only entity IDs that will be allowed to be used as + login options. See https://cilogon.org/idplist for the list of + `EntityIDs` of each IdP. - It can be used to enable domain stripping, adding prefixes to the usernames and to specify an identity provider specific username claim. + It can be used to enable domain stripping, adding prefixes to the + usernames and to specify an identity provider specific username claim. For example:: - allowed_idps = { + c.CILogonOAuthenticator.allowed_idps = { "https://idpz.utorauth.utoronto.ca/shibboleth": { "username_derivation": { "username_claim": "email", "action": "strip_idp_domain", "domain": "utoronto.ca", - } + }, }, "https://github.com/login/oauth/authorize": { "username_derivation": { "username_claim": "username", "action": "prefix", - "prefix": "gh" - } - } + "prefix": "gh", + }, + }, "http://google.com/accounts/o8/id": { "username_derivation": { "username_claim": "username", - } - "allowed_domains": ["uni.edu", "something.org"] - } + }, + "allowed_domains": ["uni.edu", "something.org"], + }, } Where `username_derivation` defines: * :attr:`username_claim`: string - The claim in the `userinfo` response from which to get the JupyterHub username. - Examples include: `eppn`, `email`. What keys are available will depend on the scopes requested. - It will overwrite any value set through CILogonOAuthenticator.username_claim for this identity provider. - * :attr:`action`: string - What action to perform on the username. Available options are "strip_idp_domain", which will strip the domain from the username if specified and "prefix", which will prefix the hub username with "prefix:". + The claim in the `userinfo` response from which to get the + JupyterHub username. Examples include: `eppn`, `email`. What + keys are available will depend on the scopes requested. It will + overwrite any value set through + CILogonOAuthenticator.username_claim for this identity provider. + * :attr:`action`: string What action to perform on the username. + Available options are "strip_idp_domain", which will strip the + domain from the username if specified and "prefix", which will + prefix the hub username with "prefix:". * :attr:`domain:` string - The domain after "@" which will be stripped from the username if it exists and if the action is "strip_idp_domain". - * :attr:`prefix`: string - The prefix which will be added at the beginning of the username followed by a semi-column ":", if the action is "prefix". - * :attr:`allowed_domains`: string - It defines which domains will be allowed to login using the specific identity provider. + The domain after "@" which will be stripped from the username if + it exists and if the action is "strip_idp_domain". + * :attr:`prefix`: string The prefix which will be added at the + beginning of the username followed by a semi-column ":", if the + action is "prefix". + * :attr:`allowed_domains`: string It defines which domains will be + allowed to login using the specific identity provider. Requirements: * if `username_derivation.action` is `strip_idp_domain`, then `username_derivation.domain` must also be specified @@ -180,16 +204,19 @@ def _validate_scope(self, proposal): def _validate_allowed_idps(self, proposal): idps = proposal.value - for entity_id, username_derivation in idps.items(): - # Validate `username_derivation` config using the schema + if not idps: + raise ValueError("One or more allowed_idps must be configured") + + for entity_id, idp_config in idps.items(): + # Validate `idp_config` config using the schema root_dir = os.path.dirname(os.path.abspath(__file__)) schema_file = os.path.join(root_dir, "schemas", "cilogon-schema.yaml") with open(schema_file) as schema_fd: schema = yaml.load(schema_fd) # Raises useful exception if validation fails - jsonschema.validate(username_derivation, schema) + jsonschema.validate(idp_config, schema) - # Make sure allowed_idps containes EntityIDs and not domain names. + # Make sure allowed_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: @@ -198,9 +225,8 @@ 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. - See https://cilogon.org/idplist for the list of EntityIDs of each IDP. - """ + "The keys of `allowed_idps` **must** be CILogon permitted EntityIDs. " + "See https://cilogon.org/idplist for the list of EntityIDs of each IDP." ) return idps @@ -208,8 +234,9 @@ def _validate_allowed_idps(self, proposal): strip_idp_domain = Bool( False, config=True, - help="""Deprecated, use `CILogonOAuthenticator.allowed_idps["username_derivation"]["action"] = "strip_idp_domain"` - to enable it and `CIlogonOAuthenticator.allowed_idps["username_derivation"]["domain"]` to list the domain + help=""" + Deprecated, use `CILogonOAuthenticator.allowed_idps[]["username_derivation"]["action"] = "strip_idp_domain"` + to enable it and `CIlogonOAuthenticator.allowed_idps[]["username_derivation"]["domain"]` to list the domain which will be stripped """, ) @@ -221,9 +248,13 @@ def _validate_allowed_idps(self, proposal): shown_idps = List( Unicode(), config=True, - help="""A list of identity providers to be shown as login options. - The `idp` attribute is the SAML Entity ID of the user's selected - identity provider. + help=""" + Deprecated, `CILogonOAuthenticator.allowed_idps` will determine the idps + shown. + + A list of identity providers to be shown as login options. The `idp` + attribute is the SAML Entity ID of the user's selected identity + provider. See https://cilogon.org/include/idplist.xml for the list of identity providers supported by CILogon. @@ -232,7 +263,8 @@ def _validate_allowed_idps(self, proposal): skin = Unicode( config=True, - help="""The `skin` attribute is the name of the custom CILogon interface skin + help=""" + The `skin` attribute is the name of the custom CILogon interface skin for your application. Contact help@cilogon.org to request a custom skin. @@ -241,144 +273,106 @@ def _validate_allowed_idps(self, proposal): additional_username_claims = List( config=True, - help="""Additional claims to check if the username_claim fails. + help=""" + Deprecated, use `CILogonOAuthenticator.allowed_idps["username_derivation"]["username_claim"]`. - This is useful for linked identities where not all of them return - the primary username_claim. + Additional claims to check if the username_claim fails. + + This is useful for linked identities where not all of them return the + primary username_claim. """, - default_value=["email"], ) - def _get_final_username_claim_list(self, user_info): + def user_info_to_username(self, user_info): """ - The username claims that will be used to determine the hub username can be set through: - - `CILogonOAutnenticator.username_claim`, that can be extended through `CILogonOAutnenticator.additional_username_claims` - or - - `CILogonOAuthenticator.allowed_idps..username_claim`, that - will overwrite any value set through CILogonOAuthenticator.username_claim - for this identity provider. - - This function returns the username claim list that will be used for the current user trying to login - based on the idp that they have selected. If no `CILogonOAutnenticator.allowed_idps` is set, then - `CILogonOAutnenticator.username_claim` will be used. + 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"]. + + Returns a username based on user_info and configuration in allowed_idps + under the associated idp's username_derivation config. """ - username_claims = [self.username_claim] - if self.additional_username_claims: - username_claims.extend(self.additional_username_claims) - if self.allowed_idps: - selected_idp = user_info["idp"] - if selected_idp in self.allowed_idps.keys(): - # The username_claim which should be used for this idp - return [ - self.allowed_idps[selected_idp]["username_derivation"][ - "username_claim" - ] - ] - else: - return username_claims - return username_claims - - def _get_username_from_claim_list(self, user_info, username_claims): - username = None - for claim in username_claims: - username = user_info.get(claim) - if username: - break + # 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. + # + 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): + message = f"Login with identity provider {user_idp} is not pre-configured" + self.log.error(message) + raise web.HTTPError(403, message) + + unprocessed_username = self._user_info_to_unprocessed_username(user_info) + username = self._get_processed_username(unprocessed_username, user_info) return username - def user_info_to_username(self, user_info): - username_claims = self._get_final_username_claim_list(user_info) - username = self._get_username_from_claim_list(user_info, username_claims) + def _user_info_to_unprocessed_username(self, user_info): + """ + Returns a username from user_info without also applying the "action" + specified under "username_derivation" for the associated idp. + """ + user_idp = user_info["idp"] + username_derivation = self.allowed_idps[user_idp]["username_derivation"] + username_claim = username_derivation["username_claim"] + username = user_info.get(username_claim) if not username: - user_info_keys = sorted(user_info.keys()) - self.log.error( - f"No username claim in the list at {username_claims} was found in the response {user_info_keys}" - ) - raise web.HTTPError(500, "Failed to get username from CILogon") - - # Optionally strip idp domain or prefix the username - if self.allowed_idps: - selected_idp = user_info["idp"] - if selected_idp in self.allowed_idps.keys(): - username_derivation = self.allowed_idps[selected_idp][ - "username_derivation" - ] - action = username_derivation.get("action") - - if action == "strip_idp_domain": - username = username.split("@", 1)[0] - elif action == "prefix": - prefix = username_derivation["prefix"] - username = f"{prefix}:{username}" + message = f"Configured username_claim {username_claim} for {user_idp} was not found in the response {user_info.keys()}" + self.log.error(message) + raise web.HTTPError(500, message) return username - async def check_allowed(self, username, auth_model): + def _get_processed_username(self, username, user_info): """ - Returns True for authorized users, raises errors for users - denied authorization. - - Overrides the `OAuthenticator.check_allowed` implementation to only allow users - logging in using a provider that is part of `allowed_idps`. - Following this, the user must either be part of `allowed_users` or `allowed_domains` - to be authorized if either is configured, otherwise all users are - authorized. + Optionally adjusts a username from user_info based on the "action" + specified under "username_derivation" for the associated idp. """ - # A workaround for JupyterHub<=4.0.1, described in - # https://github.com/jupyterhub/oauthenticator/issues/621 - if auth_model is None: - return True + user_idp = user_info["idp"] + username_derivation = self.allowed_idps[user_idp]["username_derivation"] + + # Optionally execute action "strip_idp_domain" or "prefix" + action = username_derivation.get("action") + if action == "strip_idp_domain": + domain_suffix = "@" + username_derivation["domain"] + if username.lower().endswith(domain_suffix.lower()): + username = username[: -len(domain_suffix)] + elif action == "prefix": + prefix = username_derivation["prefix"] + username = f"{prefix}:{username}" - # allow admin users recognized via admin_users or update_auth_model - if auth_model["admin"]: - return True + return username - if self.allowed_idps: - user_info = auth_model["auth_state"][self.user_auth_state_key] - selected_idp = user_info["idp"] - if selected_idp not in self.allowed_idps.keys(): - self.log.error( - f"Trying to login from an identity provider that was not allowed {selected_idp}", - ) - raise web.HTTPError( - 403, - "Trying to login using an identity provider that was not allowed", - ) + async def check_allowed(self, username, auth_model): + """ + Overrides the OAuthenticator.check_allowed to also allow users part of + an `allowed_domains` as configured under `allowed_idps`. + """ + if await super().check_allowed(username, auth_model): + return True - allowed_domains = self.allowed_idps[selected_idp].get("allowed_domains") - if self.allowed_users or allowed_domains: - if username in self.allowed_users: - return True - - if allowed_domains: - username_claims = self._get_final_username_claim_list(user_info) - username_with_domain = self._get_username_from_claim_list( - user_info, username_claims - ) - user_domain = username_with_domain.split("@", 1)[1] - if user_domain in allowed_domains: - return True - else: - raise web.HTTPError( - 403, - "Trying to login using a domain that was not allowed", - ) - - return False - # Although not recommended, it might be that `allowed_idps` is not specified - # In this case we need to make sure we still check `allowed_users` and don't assume - # everyone should be authorized - elif self.allowed_users: - if username in self.allowed_users: + user_info = auth_model["auth_state"][self.user_auth_state_key] + user_idp = user_info["idp"] + idp_allowed_domains = self.allowed_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() + if user_domain in idp_allowed_domains: return True - return False - # otherwise, authorize all users - return True + message = f"Login with domain @{user_domain} is not allowed" + self.log.warning(message) + raise web.HTTPError(403, message) + # users should be explicitly allowed via config, otherwise they aren't + return False -class LocalCILogonOAuthenticator(LocalAuthenticator, CILogonOAuthenticator): +class LocalCILogonOAuthenticator(LocalAuthenticator, CILogonOAuthenticator): """A version that mixes in local system user creation""" diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py index cae41193..18e552e8 100644 --- a/oauthenticator/generic.py +++ b/oauthenticator/generic.py @@ -84,16 +84,15 @@ def _default_http_client(self): ) def user_info_to_username(self, user_info): + """ + Overrides OAuthenticator.user_info_to_username to support the + GenericOAuthenticator unique feature of allowing username_claim to be a + callable function. + """ if callable(self.username_claim): - username = self.username_claim(user_info) + return self.username_claim(user_info) else: - username = user_info.get(self.username_claim, None) - if not username: - message = (f"No {self.username_claim} found in {user_info}",) - self.log.error(message) - raise ValueError(message) - - return username + return super().user_info_to_username(user_info) def get_user_groups(self, user_info): """ @@ -140,34 +139,20 @@ async def update_auth_model(self, auth_model): async def check_allowed(self, username, auth_model): """ - Returns True for users allowed to be authorized. - - Overrides the OAuthenticator.check_allowed implementation to allow users - either part of `allowed_users` or `allowed_groups`, and not just those - part of `allowed_users`. + Overrides the OAuthenticator.check_allowed to also allow users part of + `allowed_groups`. """ - # A workaround for JupyterHub<=4.0.1, described in - # https://github.com/jupyterhub/oauthenticator/issues/621 - if auth_model is None: + if await super().check_allowed(username, auth_model): return True - # allow admin users recognized via admin_users or update_auth_model - if auth_model["admin"]: - return True - - # if allowed_users or allowed_groups is configured, we deny users not - # part of either - if self.allowed_users or self.allowed_groups: + if self.allowed_groups: user_info = auth_model["auth_state"][self.user_auth_state_key] user_groups = self.get_user_groups(user_info) - if username in self.allowed_users: - return True if any(user_groups & self.allowed_groups): return True - return False - # otherwise, authorize all users - return True + # users should be explicitly allowed via config, otherwise they aren't + return False class LocalGenericOAuthenticator(LocalAuthenticator, GenericOAuthenticator): diff --git a/oauthenticator/github.py b/oauthenticator/github.py index 47ba4769..86bf3b84 100644 --- a/oauthenticator/github.py +++ b/oauthenticator/github.py @@ -133,46 +133,25 @@ def _github_client_secret_changed(self, name, old, new): async def check_allowed(self, username, auth_model): """ - Returns True for users allowed to be authorized. - - Overrides the OAuthenticator.check_allowed implementation to allow users - either part of `allowed_users` or `allowed_organizations`, and not just - those part of `allowed_users`. + Overrides the OAuthenticator.check_allowed to also allow users part of + `allowed_organizations`. """ - # A workaround for JupyterHub<=4.0.1, described in - # https://github.com/jupyterhub/oauthenticator/issues/621 - if auth_model is None: - return True - - # allow admin users recognized via admin_users or update_auth_model - if auth_model["admin"]: + if await super().check_allowed(username, auth_model): return True - # if allowed_users or allowed_organizations is configured, we deny users not - # part of either - if self.allowed_users or self.allowed_organizations: - if username in self.allowed_users: - return True - - if self.allowed_organizations: - access_token = auth_model["auth_state"]["token_response"][ - "access_token" - ] - token_type = auth_model["auth_state"]["token_response"]["token_type"] - for org in self.allowed_organizations: - user_in_org = await self._check_membership_allowed_organizations( - org, username, access_token, token_type - ) - if user_in_org: - return True - self.log.warning( - f"User {username} is not part of allowed_organizations" - ) - - return False - - # otherwise, authorize all users - return True + if self.allowed_organizations: + access_token = auth_model["auth_state"]["token_response"]["access_token"] + token_type = auth_model["auth_state"]["token_response"]["token_type"] + for org_team in self.allowed_organizations: + if await self._check_membership_allowed_organizations( + org_team, username, access_token, token_type + ): + return True + message = f"User {username} is not part of allowed_organizations" + self.log.warning(message) + + # users should be explicitly allowed via config, otherwise they aren't + return False async def update_auth_model(self, auth_model): """ diff --git a/oauthenticator/gitlab.py b/oauthenticator/gitlab.py index e2d36048..f9929e32 100644 --- a/oauthenticator/gitlab.py +++ b/oauthenticator/gitlab.py @@ -122,48 +122,31 @@ async def _set_gitlab_version(self, access_token): async def check_allowed(self, username, auth_model): """ - Returns True for users allowed to be authorized. - - Overrides the OAuthenticator.check_allowed implementation to allow users - either part of `allowed_users`, `allowed_gitlab_groups`, or `allowed_project_ids`, - and not just those part of `allowed_users`. + Overrides the OAuthenticator.check_allowed to also allow users part of + `allowed_google_groups` or `allowed_project_ids`. """ - # A workaround for JupyterHub<=4.0.1, described in - # https://github.com/jupyterhub/oauthenticator/issues/621 - if auth_model is None: + if await super().check_allowed(username, auth_model): return True - # allow admin users recognized via admin_users or update_auth_model - if auth_model["admin"]: - return True + access_token = auth_model["auth_state"]["token_response"]["access_token"] + user_id = auth_model["auth_state"][self.user_auth_state_key]["id"] - # if allowed_users, allowed_gitlab_groups, or allowed_project_ids is - # configured, we deny users not part of either - if self.allowed_users or self.allowed_gitlab_groups or self.allowed_project_ids: - if username in self.allowed_users: + if self.allowed_gitlab_groups: + user_in_group = await self._check_membership_allowed_groups( + user_id, access_token + ) + if user_in_group: return True - access_token = auth_model["auth_state"]["token_response"]["access_token"] - user_id = auth_model["auth_state"][self.user_auth_state_key]["id"] - - if self.allowed_gitlab_groups: - user_in_group = await self._check_membership_allowed_groups( - user_id, access_token - ) - if user_in_group: - return True - - if self.allowed_project_ids: - user_in_project = await self._check_membership_allowed_project_ids( - user_id, access_token - ) - if user_in_project: - return True - - return False + if self.allowed_project_ids: + user_in_project = await self._check_membership_allowed_project_ids( + user_id, access_token + ) + if user_in_project: + return True - # otherwise, authorize all users - return True + # users should be explicitly allowed via config, otherwise they aren't + return False async def _get_gitlab_version(self, access_token): url = f"{self.gitlab_api}/version" diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py index 4e225685..f23d9bc0 100644 --- a/oauthenticator/globus.py +++ b/oauthenticator/globus.py @@ -7,7 +7,7 @@ import urllib from jupyterhub.auth import LocalAuthenticator -from tornado.web import HTTPError +from tornado import web from traitlets import Bool, List, Set, Unicode, default from .oauth2 import OAuthenticator, OAuthLogoutHandler @@ -91,20 +91,28 @@ def _token_url_default(self): ) identity_provider = Unicode( - help="""Restrict which institution a user - can use to login (GlobusID, University of Hogwarts, etc.). This should - be set in the app at developers.globus.org, but this acts as an additional - check to prevent unnecessary account creation.""" - ).tag(config=True) + config=True, + help=""" + Restrict which institution (domain) a user can use to login (GlobusID, + University of Hogwarts, etc.). This should be set in the app at + developers.globus.org, but this acts as an additional check to prevent + unnecessary account creation. + + Note that users with an associated email domains must still be allowed + via another config, such as `allow_all`. + """, + ) def _identity_provider_default(self): return os.getenv('IDENTITY_PROVIDER', '') username_from_email = Bool( False, - help="""Create username from email address, not preferred username. If - an identity provider is specified, email address must be from the same - domain. Email scope will be set automatically.""", + help=""" + Create username from email address, not preferred username. If an + identity provider is specified, email address must be from the same + domain. Email scope will be set automatically. + """, config=True, ) @@ -115,9 +123,12 @@ def _username_claim_default(self): return "preferred_username" exclude_tokens = List( - help="""Exclude tokens from being passed into user environments - when they start notebooks, Terminals, etc.""" - ).tag(config=True) + config=True, + help=""" + Exclude tokens from being passed into user environments when they start + notebooks, Terminals, etc. + """, + ) def _exclude_tokens_default(self): return ['auth.globus.org', 'groups.api.globus.org'] @@ -137,33 +148,50 @@ def _scope_default(self): return scopes globus_local_endpoint = Unicode( - help="""If Jupyterhub is also a Globus - endpoint, its endpoint id can be specified here.""" - ).tag(config=True) + config=True, + help=""" + If JupyterHub is also a Globus endpoint, its endpoint id can be + specified here. + """, + ) def _globus_local_endpoint_default(self): return os.getenv('GLOBUS_LOCAL_ENDPOINT', '') revoke_tokens_on_logout = Bool( - help="""Revoke tokens so they cannot be used again. Single-user servers - MUST be restarted after logout in order to get a fresh working set of - tokens.""" - ).tag(config=True) + config=True, + help=""" + Revoke tokens so they cannot be used again. Single-user servers MUST be + restarted after logout in order to get a fresh working set of tokens. + """, + ) def _revoke_tokens_on_logout_default(self): return False allowed_globus_groups = Set( - help="""Allow members of defined Globus Groups to access JupyterHub. Users in an - admin Globus Group are also automatically allowed. Groups are specified with their UUIDs. Setting this will - add the Globus Groups scope.""" - ).tag(config=True) + config=True, + help=""" + Allow members of defined Globus Groups, specified with their UUIDs, to + login. + + If this is configured, the default value of the scope configuration is + appended with the scope + `urn:globus:auth:scope:groups.api.globus.org:view_my_groups_and_memberships`. + """, + ) admin_globus_groups = Set( - help="""Set members of defined Globus Groups as JupyterHub admin users. - These users are automatically allowed to login to JupyterHub. Groups are specified with - their UUIDs. Setting this will add the Globus Groups scope.""" - ).tag(config=True) + config=True, + help=""" + Allow members of defined Globus Groups, specified with their UUIDs, to + login as admin users. + + If this is configured, the default value of the scope configuration is + appended with the scope + `urn:globus:auth:scope:groups.api.globus.org:view_my_groups_and_memberships`. + """, + ) async def pre_spawn_start(self, user, spawner): """Add tokens to the spawner whenever the spawner starts a notebook. @@ -208,7 +236,6 @@ def build_auth_state_dict(self, token_info, user_info): accounts) will correspond to a Globus User ID, so foouser@globusid.org will have the 'foouser' account in Jupyterhub. """ - tokens = self.get_globus_tokens(token_info) # historically, tokens have been organized by resource server for convenience. # If multiple scopes are requested from the same resource server, they will be @@ -246,51 +273,33 @@ async def _fetch_users_groups(self, tokens): async def check_allowed(self, username, auth_model): """ - Returns True for users allowed to be authorized. - - Overrides the OAuthenticator.check_allowed implementation to allow users - either part of `allowed_users` or `allowed_globus_groups`, and not just those - part of `allowed_users`. + Overrides the OAuthenticator.check_allowed to also allow users part of + `allowed_globus_groups`. """ - # A workaround for JupyterHub<=4.0.1, described in - # https://github.com/jupyterhub/oauthenticator/issues/621 - if auth_model is None: - return True - - # allow admin users recognized via admin_users or update_auth_model - if auth_model["admin"]: - return True - + # before considering allowing a username by being recognized in a list + # of usernames or similar, we must ensure that the authenticated user is + # from an allowed identity provider domain. if self.identity_provider: # It's possible for identity provider domains to be namespaced # https://docs.globus.org/api/auth/specification/#identity_provider_namespaces user_info = auth_model["auth_state"][self.user_auth_state_key] - domain = user_info.get(self.username_claim).split('@', 1)[-1] - if domain != self.identity_provider: - self.log.warning( - f"Trying to login from an identity provider that was not allowed {domain}", - ) - raise HTTPError( - 403, - f"This site is restricted to {self.identity_provider} accounts. " - "Please link your account at app.globus.org/account.", - ) + user_domain = user_info.get(self.username_claim).split('@', 1)[-1] + if user_domain != self.identity_provider: + message = f"This site is restricted to {self.identity_provider} accounts. Link your account at app.globus.org/account." + self.log.warning(message) + raise web.HTTPError(403, message) - # if allowed_users or allowed_globus_groups is configured, we deny users - # not part of either - if self.allowed_users or self.allowed_globus_groups: - if username in self.allowed_users: - return True - if self.allowed_globus_groups: - user_groups = auth_model["auth_state"]["globus_groups"] - if any(user_groups & self.allowed_globus_groups): - return True - self.log.warning(f"{username} not in an allowed Globus Group") + if await super().check_allowed(username, auth_model): + return True - return False + if self.allowed_globus_groups: + user_groups = auth_model["auth_state"]["globus_groups"] + if any(user_groups & self.allowed_globus_groups): + return True + self.log.warning(f"{username} not in an allowed Globus Group") - # otherwise, authorize all users - return True + # users should be explicitly allowed via config, otherwise they aren't + return False async def update_auth_model(self, auth_model): """ @@ -298,9 +307,9 @@ async def update_auth_model(self, auth_model): or `admin_globus_groups` is configured. Sets admin status to True or False if `admin_globus_groups` is - configured and the user isn't part of `admin_users` or - `admin_globus_groups`. Note that leaving it at None makes users able to - retain an admin status while setting it to False makes it be revoked. + configured and the user isn't part of `admin_users`. Note that leaving + it at None makes users able to retain an admin status while setting it + to False makes it be revoked. """ user_groups = set() if self.allowed_globus_groups or self.admin_globus_groups: @@ -324,7 +333,6 @@ def user_info_to_username(self, user_info): accounts) will correspond to a Globus User ID, so foouser@globusid.org will have the 'foouser' account in Jupyterhub. """ - return user_info.get(self.username_claim).split('@')[0] def get_default_headers(self): @@ -352,7 +360,6 @@ async def revoke_service_tokens(self, services): ... } """ - access_tokens = [ token_dict.get('access_token') for token_dict in services.values() ] diff --git a/oauthenticator/google.py b/oauthenticator/google.py index f09d9634..e94b8490 100644 --- a/oauthenticator/google.py +++ b/oauthenticator/google.py @@ -85,7 +85,13 @@ def _userdata_url_default(self): hosted_domain = List( Unicode(), config=True, - help="""List of domains used to restrict sign-in, e.g. mycollege.edu""", + help=""" + Restrict sign-in to a list of email domain names, such as + `["mycollege.edu"]`. + + Note that users with email domains in this list must still be allowed + via another config, such as `allow_all`. + """, ) @default('hosted_domain') @@ -107,8 +113,8 @@ def _cast_hosted_domain(self, proposal): # (or if it's empty, an empty list) if proposal.value == '': return [] - return [proposal.value] - return proposal.value + return [proposal.value.lower()] + return [hd.lower() for hd in proposal.value] login_service = Unicode( os.environ.get('LOGIN_SERVICE', 'Google'), @@ -119,17 +125,16 @@ def _cast_hosted_domain(self, proposal): async def update_auth_model(self, auth_model): """ Fetch and store `google_groups` in auth state if `allowed_google_groups` - or `admin_google_groups` is configured. Also declare the user an admin - if part of `admin_google_groups`. + or `admin_google_groups` is configured. Sets admin status to True or False if `admin_google_groups` is - configured and the user isn't part of `admin_users` or - `admin_google_groups`. Note that leaving it at None makes users able to - retain an admin status while setting it to False makes it be revoked. + configured and the user isn't part of `admin_users`. Note that leaving + it at None makes users able to retain an admin status while setting it + to False makes it be revoked. """ user_info = auth_model["auth_state"][self.user_auth_state_key] user_email = user_info["email"] - user_domain = user_info["domain"] = user_email.split("@")[1] + user_domain = user_info["domain"] = user_email.split("@")[1].lower() user_groups = set() if self.allowed_google_groups or self.admin_google_groups: @@ -151,29 +156,20 @@ async def update_auth_model(self, auth_model): async def check_allowed(self, username, auth_model): """ - Returns True for users allowed to be authorized. - - Overrides the OAuthenticator.check_allowed implementation to allow users - either part of `allowed_users` or `allowed_google_groups`, and not just those - part of `allowed_users`. + Overrides the OAuthenticator.check_allowed to also allow users part of + `allowed_google_groups`. """ - # A workaround for JupyterHub<=4.0.1, described in - # https://github.com/jupyterhub/oauthenticator/issues/621 - if auth_model is None: - return True - - # allow admin users recognized via admin_users or update_auth_model - if auth_model["admin"]: - return True - + # before considering allowing a username by being recognized in a list + # of usernames or similar, we must ensure that the authenticated user + # has a verified email and is part of hosted_domain if configured. user_info = auth_model["auth_state"][self.user_auth_state_key] user_email = user_info["email"] user_domain = user_info["domain"] - user_groups = user_info["google_groups"] if not user_info["verified_email"]: - self.log.warning(f"Google OAuth unverified email attempt: {user_email}") - raise HTTPError(403, f"Google email {user_email} not verified") + message = f"Login with unverified email {user_email} is not allowed" + self.log.warning(message) + raise HTTPError(403, message) # NOTE: If hosted_domain is configured as ["a.com", "b.com"], and # allowed_google_groups is declared as {"a.com": {"a-group"}}, a @@ -184,26 +180,21 @@ async def check_allowed(self, username, auth_model): # if self.hosted_domain: if user_domain not in self.hosted_domain: - self.log.warning( - f"Google OAuth unauthorized domain attempt: {user_email}" - ) - raise HTTPError( - 403, f"Google account domain @{user_domain} not authorized" - ) - - # if allowed_users or allowed_google_groups is configured, we deny users - # not part of either - if self.allowed_users or self.allowed_google_groups: - if username in self.allowed_users: + message = f"Login with domain @{user_domain} is not allowed" + self.log.warning(message) + raise HTTPError(403, message) + + if await super().check_allowed(username, auth_model): + return True + + if self.allowed_google_groups: + user_groups = user_info["google_groups"] + allowed_groups = self.allowed_google_groups.get(user_domain, set()) + if any(user_groups & allowed_groups): return True - if self.allowed_google_groups: - allowed_groups = self.allowed_google_groups.get(user_domain, set()) - if any(user_groups & allowed_groups): - return True - return False - - # otherwise, authorize all users - return True + + # users should be explicitly allowed via config, otherwise they aren't + return False def _service_client_credentials(self, scopes, user_email_domain): """ diff --git a/oauthenticator/mediawiki.py b/oauthenticator/mediawiki.py index 49caa90b..d1bff3b9 100644 --- a/oauthenticator/mediawiki.py +++ b/oauthenticator/mediawiki.py @@ -80,6 +80,7 @@ class MWOAuthenticator(OAuthenticator): login_service = 'MediaWiki' login_handler = MWLoginHandler callback_handler = MWCallbackHandler + user_auth_state_key = "MEDIAWIKI_USER_IDENTITY" mw_index_url = Unicode( os.environ.get('MW_INDEX_URL', 'https://meta.wikimedia.org/w/index.php'), @@ -144,5 +145,5 @@ def build_auth_state_dict(self, token_info, user_info): return { 'ACCESS_TOKEN_KEY': token_info["access_token"].key, 'ACCESS_TOKEN_SECRET': token_info["access_token"].secret, - 'MEDIAWIKI_USER_IDENTITY': user_info, + self.user_auth_state_key: user_info, } diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py index 564cb2eb..a703c378 100644 --- a/oauthenticator/oauth2.py +++ b/oauthenticator/oauth2.py @@ -255,6 +255,16 @@ class OAuthenticator(Authenticator): # To be overridden by each oauthenticator user_auth_state_key = "oauth_user" + allow_all = Bool( + False, + config=True, + help=""" + Allow all authenticated users to login. + + .. versionadded:: 16.0 + """, + ) + authorize_url = Unicode( config=True, help="""The authenticate url for initiating oauth""" ) @@ -284,11 +294,14 @@ def _userdata_url_default(self): username_claim = Unicode( "username", config=True, - help="""Field in userdata reply to use for username - The field in the userdata response from which to get the JupyterHub username. + help=""" + Field in userdata reply to use for username The field in the userdata + response from which to get the JupyterHub username. + Examples include: email, username, nickname - What keys are available will depend on the scopes requested and the authenticator used. + What keys are available will depend on the scopes requested and the + authenticator used. """, ) @@ -802,25 +815,30 @@ async def check_allowed(self, username, auth_model): `OAuthenticator.authenticate` has been called, and therefore also after `update_auth_model` has been called. - Subclasses with authorization logic involving allowed groups should - override this. + Subclasses with additional config to allow a user should override this + method and return True when this method returns True or if a user is + allowed via the additional config. """ # A workaround for JupyterHub<=4.0.1, described in # https://github.com/jupyterhub/oauthenticator/issues/621 if auth_model is None: return True - # authorize users to become admins by admin_users or logic in - # update_auth_model + if self.allow_all: + return True + + # allow users with admin status set to True via admin_users config or + # update_auth_model override if auth_model["admin"]: return True - # if allowed_users is configured, authorize/unauthorize based on that - if self.allowed_users: - return username in self.allowed_users + # allow users in allowed_users, note that allowed_users is appended + # automatically with existing users if it was configured truthy + if username in self.allowed_users: + return True - # otherwise, authorize all users - return True + # users should be explicitly allowed via config, otherwise they aren't + return False _deprecated_oauth_aliases = {} diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py index c876a7cf..5dd7a470 100644 --- a/oauthenticator/openshift.py +++ b/oauthenticator/openshift.py @@ -87,17 +87,25 @@ def _userdata_url_default(self): return f"{self.openshift_rest_api_url}/apis/user.openshift.io/v1/users/~" def user_info_to_username(self, user_info): + """ + Overrides OAuthenticator.user_info_to_username instead of setting + username_claim as the username is nested inside another dictionary. + """ return user_info['metadata']['name'] async def update_auth_model(self, auth_model): """ - Update admin status based on `admin_groups` if its configured. + Sets admin status to True or False if `admin_groups` is configured and + the user isn't part of `admin_users`. Note that leaving it at None makes + users able to retain an admin status while setting it to False makes it + be revoked. """ + if auth_model["admin"]: + # auth_model["admin"] being True means the user was in admin_users + return auth_model + if self.admin_groups: - # if admin_groups is configured and the user wasn't part of - # admin_users, we must set the admin status to True or False, - # otherwise removing a user from the admin_groups won't have an - # effect + # admin status should in this case be True or False, not None user_info = auth_model["auth_state"][self.user_auth_state_key] user_groups = set(user_info["groups"]) auth_model["admin"] = any(user_groups & self.admin_groups) @@ -106,34 +114,20 @@ async def update_auth_model(self, auth_model): async def check_allowed(self, username, auth_model): """ - Returns True for users allowed to be authorized. - - Overrides the OAuthenticator.check_allowed implementation to allow users - either part of `allowed_users` or `allowed_groups`, and not just those - part of `allowed_users`. + Overrides OAuthenticator.check_allowed to also allow users part of + `allowed_groups`. """ - # A workaround for JupyterHub<=4.0.1, described in - # https://github.com/jupyterhub/oauthenticator/issues/621 - if auth_model is None: + if await super().check_allowed(username, auth_model): return True - # allow admin users recognized via admin_users or update_auth_model - if auth_model["admin"]: - return True - - # if allowed_users or allowed_groups is configured, we deny users not - # part of either - if self.allowed_users or self.allowed_groups: - if username in self.allowed_users: - return True + if self.allowed_groups: user_info = auth_model["auth_state"][self.user_auth_state_key] user_groups = set(user_info["groups"]) if any(user_groups & self.allowed_groups): return True - return False - # otherwise, authorize all users - return True + # users should be explicitly allowed via config, otherwise they aren't + return False class LocalOpenShiftOAuthenticator(LocalAuthenticator, OpenShiftOAuthenticator): diff --git a/oauthenticator/schemas/cilogon-schema.yaml b/oauthenticator/schemas/cilogon-schema.yaml index a15dd4af..713d2bd2 100644 --- a/oauthenticator/schemas/cilogon-schema.yaml +++ b/oauthenticator/schemas/cilogon-schema.yaml @@ -1,5 +1,7 @@ +# This JSONSchema is used to validate the values in the +# CILogonOAuthenticator.allowed_idps dictionary. +# $schema: http://json-schema.org/draft-07/schema# -title: username_derivation type: object additionalProperties: false required: @@ -12,6 +14,8 @@ properties: username_derivation: type: object additionalProperties: false + required: + - username_claim properties: username_claim: type: string @@ -24,9 +28,8 @@ properties: type: string prefix: type: string - required: - - username_claim allOf: + # if action is strip_idp_domain, then domain is required - if: properties: action: @@ -36,6 +39,7 @@ properties: then: required: - domain + # if action is prefix, then prefix is required - if: properties: action: diff --git a/oauthenticator/tests/test_auth0.py b/oauthenticator/tests/test_auth0.py index 7fb28ef4..4fd81437 100644 --- a/oauthenticator/tests/test_auth0.py +++ b/oauthenticator/tests/test_auth0.py @@ -1,7 +1,7 @@ import logging from unittest.mock import Mock -from pytest import fixture, mark +from pytest import fixture, mark, raises from tornado import web from traitlets.config import Config @@ -9,58 +9,89 @@ from ..oauth2 import OAuthLogoutHandler from .mocks import mock_handler, setup_oauth_mock -auth0_subdomain = "jupyterhub-test" -auth0_domain = "jupyterhub-test.auth0.com" - - -def user_model(email, nickname=None): - """Return a user model""" - return { - 'email': email, - 'nickname': nickname if nickname else email, - 'name': 'Hoban Washburn', - } +AUTH0_DOMAIN = "jupyterhub-test.auth0.com" @fixture def auth0_client(client): setup_oauth_mock( client, - host=auth0_domain, + host=AUTH0_DOMAIN, access_token_path='/oauth/token', user_path='/userinfo', ) return client -@mark.parametrize( - 'config', [{"auth0_domain": auth0_domain}, {"auth0_subdomain": auth0_subdomain}] -) -async def test_auth0(config, auth0_client): - cfg = Config() - cfg.Auth0OAuthenticator = Config(config) - authenticator = Auth0OAuthenticator(config=cfg) - - handler = auth0_client.handler_for_user(user_model('kaylee@serenity.now')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert sorted(auth_model) == ['admin', 'auth_state', 'name'] - assert auth_model['name'] == 'kaylee@serenity.now' - auth_state = auth_model['auth_state'] - assert 'access_token' in auth_state - assert 'auth0_user' in auth_state +def user_model(): + """Return a user model""" + return { + "email": "user1@example.com", + "name": "user1", + } @mark.parametrize( - 'config', [{"auth0_domain": auth0_domain}, {"auth0_subdomain": auth0_subdomain}] + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "admin_users": {"user1"}, + "allowed_users": {"not-test-user"}, + }, + True, + True, + ), + ], ) -async def test_username_key(config, auth0_client): - cfg = Config() - cfg.Auth0OAuthenticator = Config(config) - authenticator = Auth0OAuthenticator(config=cfg) - authenticator.username_key = 'nickname' - handler = auth0_client.handler_for_user(user_model('kaylee@serenity.now', 'kayle')) +async def test_auth0( + auth0_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.Auth0OAuthenticator = Config(class_config) + c.Auth0OAuthenticator.auth0_domain = AUTH0_DOMAIN + c.Auth0OAuthenticator.username_claim = "name" + authenticator = Auth0OAuthenticator(config=c) + + handled_user_model = user_model() + handler = auth0_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'kayle' + + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "access_token" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert user_info == handled_user_model + assert auth_model["name"] == user_info[authenticator.username_claim] + else: + assert auth_model == None async def test_custom_logout(monkeypatch): @@ -79,23 +110,51 @@ async def test_custom_logout(monkeypatch): assert authenticator.logout_url('http://myhost') == 'http://myhost/logout' # Check redirection to the custom logout url - authenticator.auth0_domain = auth0_domain + authenticator.auth0_domain = AUTH0_DOMAIN await logout_handler.get() - custom_logout_url = f'https://{auth0_domain}/v2/logout' + custom_logout_url = f'https://{AUTH0_DOMAIN}/v2/logout' logout_handler.redirect.assert_called_with(custom_logout_url) -async def test_deprecated_config(caplog): - cfg = Config() - cfg.Auth0OAuthenticator.username_key = 'nickname' - log = logging.getLogger("testlog") - authenticator = Auth0OAuthenticator(config=cfg, log=log) - - assert ( - log.name, - logging.WARNING, - 'Auth0OAuthenticator.username_key is deprecated in Auth0OAuthenticator 16.0.0, use ' - 'Auth0OAuthenticator.username_claim instead', - ) in caplog.record_tuples - - assert authenticator.username_claim == 'nickname' +@mark.parametrize( + "test_variation_id,class_config,expect_config,expect_loglevel,expect_message", + [ + ( + "username_key", + {"username_key": "dummy"}, + {"username_claim": "dummy"}, + logging.WARNING, + "Auth0OAuthenticator.username_key is deprecated in Auth0OAuthenticator 16.0.0, use Auth0OAuthenticator.username_claim instead", + ), + ], +) +async def test_deprecated_config( + caplog, + test_variation_id, + class_config, + expect_config, + expect_loglevel, + expect_message, +): + """ + Tests that a warning is emitted when using a deprecated config and that + configuring the old config ends up configuring the new config. + """ + print(f"Running test variation id {test_variation_id}") + c = Config() + c.Auth0OAuthenticator = Config(class_config) + + test_logger = logging.getLogger('testlog') + if expect_loglevel == logging.ERROR: + with raises(ValueError, match=expect_message): + Auth0OAuthenticator(config=c, log=test_logger) + else: + authenticator = Auth0OAuthenticator(config=c, log=test_logger) + for key, value in expect_config.items(): + assert getattr(authenticator, key) == value + + captured_log_tuples = caplog.record_tuples + print(captured_log_tuples) + + expected_log_tuple = (test_logger.name, expect_loglevel, expect_message) + assert expected_log_tuple in captured_log_tuples diff --git a/oauthenticator/tests/test_azuread.py b/oauthenticator/tests/test_azuread.py index 0070540a..de41cdca 100644 --- a/oauthenticator/tests/test_azuread.py +++ b/oauthenticator/tests/test_azuread.py @@ -6,23 +6,27 @@ from unittest import mock import jwt -import pytest +from pytest import fixture, mark from traitlets.config import Config from ..azuread import AzureAdOAuthenticator from .mocks import setup_oauth_mock -async def test_tenant_id_from_env(): - tenant_id = "some_random_id" - with mock.patch.dict(os.environ, {"AAD_TENANT_ID": tenant_id}): - aad = AzureAdOAuthenticator() - assert aad.tenant_id == tenant_id +@fixture +def azure_client(client): + setup_oauth_mock( + client, + host=['login.microsoftonline.com'], + access_token_path=re.compile('^/[^/]+/oauth2/token$'), + token_request_style='jwt', + ) + return client def user_model(tenant_id, client_id, name): """Return a user model""" - # model derived from https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens#v20 + # id_token derived from https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens#v20 now = int(time.time()) id_token = jwt.encode( { @@ -49,62 +53,95 @@ def user_model(tenant_id, client_id, name): } -@pytest.fixture -def azure_client(client): - setup_oauth_mock( - client, - host=['login.microsoftonline.com'], - access_token_path=re.compile('^/[^/]+/oauth2/token$'), - token_request_style='jwt', - ) - return client - - -@pytest.mark.parametrize( - 'username_claim', +@mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", [ - None, - 'name', - 'oid', - 'preferred_username', + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "admin_users": {"user1"}, + "allowed_users": {"not-test-user"}, + }, + True, + True, + ), + # test username_claim + ( + "20", + {"allow_all": True, "username_claim": "name"}, + True, + None, + ), + ( + "21", + {"allow_all": True, "username_claim": "oid"}, + True, + None, + ), + ( + "22", + {"allow_all": True, "username_claim": "preferred_username"}, + True, + None, + ), ], ) -async def test_azuread(username_claim, azure_client): - cfg = Config() - cfg.AzureAdOAuthenticator = Config( - { - "tenant_id": str(uuid.uuid1()), - "client_id": str(uuid.uuid1()), - "client_secret": str(uuid.uuid1()), - } - ) - - if username_claim: - cfg.AzureAdOAuthenticator.username_claim = username_claim - - authenticator = AzureAdOAuthenticator(config=cfg) - - handler = azure_client.handler_for_user( - user_model( - tenant_id=authenticator.tenant_id, - client_id=authenticator.client_id, - name="somebody", - ) +async def test_azuread( + azure_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.AzureAdOAuthenticator = Config(class_config) + c.AzureAdOAuthenticator.tenant_id = str(uuid.uuid1()) + c.AzureAdOAuthenticator.client_id = str(uuid.uuid1()) + c.AzureAdOAuthenticator.client_secret = str(uuid.uuid1()) + authenticator = AzureAdOAuthenticator(config=c) + + handled_user_model = user_model( + tenant_id=authenticator.tenant_id, + client_id=authenticator.client_id, + name="user1", ) + handler = azure_client.handler_for_user(handled_user_model) + auth_model = await authenticator.get_authenticated_user(handler, None) + + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "access_token" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert user_info["aud"] == authenticator.client_id + assert auth_model["name"] == user_info[authenticator.username_claim] + else: + assert auth_model == None - auth_model = await authenticator.authenticate(handler) - assert sorted(auth_model) == ['admin', 'auth_state', 'name'] - - auth_state = auth_model['auth_state'] - assert 'access_token' in auth_state - assert 'user' in auth_state - - auth_state_user_info = auth_state['user'] - assert auth_state_user_info['aud'] == authenticator.client_id - username = auth_model['name'] - if username_claim: - assert username == auth_state_user_info[username_claim] - else: - # The default AzureADOAuthenticator `username_claim` is "name" - assert username == auth_state_user_info["name"] +async def test_tenant_id_from_env(): + tenant_id = "some_random_id" + with mock.patch.dict(os.environ, {"AAD_TENANT_ID": tenant_id}): + aad = AzureAdOAuthenticator() + assert aad.tenant_id == tenant_id diff --git a/oauthenticator/tests/test_bitbucket.py b/oauthenticator/tests/test_bitbucket.py index 1cf24b4f..d6fe5deb 100644 --- a/oauthenticator/tests/test_bitbucket.py +++ b/oauthenticator/tests/test_bitbucket.py @@ -1,19 +1,12 @@ import logging -from pytest import fixture +from pytest import fixture, mark, raises from traitlets.config import Config from ..bitbucket import BitbucketOAuthenticator from .mocks import setup_oauth_mock -def user_model(username): - """Return a user model""" - return { - 'username': username, - } - - @fixture def bitbucket_client(client): setup_oauth_mock( @@ -22,74 +15,148 @@ def bitbucket_client(client): access_token_path='/site/oauth2/access_token', user_path='/2.0/user', ) - return client - - -async def test_bitbucket(bitbucket_client): - authenticator = BitbucketOAuthenticator() - handler = bitbucket_client.handler_for_user(user_model('yorba')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert sorted(auth_model) == ['admin', 'auth_state', 'name'] - assert auth_model['name'] == 'yorba' - auth_state = auth_model['auth_state'] - assert 'access_token' in auth_state - assert 'bitbucket_user' in auth_state - - -async def test_allowed_teams(bitbucket_client): - client = bitbucket_client - authenticator = BitbucketOAuthenticator() - authenticator.allowed_teams = ['blue'] - teams = { - 'red': ['grif', 'simmons', 'donut', 'sarge', 'lopez'], - 'blue': ['tucker', 'caboose', 'burns', 'sheila', 'texas'], + # mock separate REST API used to check team membership + team_members = { + "group1": ["user1"], } def list_teams(request): token = request.headers['Authorization'].split(None, 1)[1] username = client.access_tokens[token]['username'] values = [] - for team, members in teams.items(): + for team, members in team_members.items(): if username in members: values.append({'name': team}) return {'values': values} - client.hosts['api.bitbucket.org'].append(('/2.0/workspaces', list_teams)) + client.hosts["api.bitbucket.org"].append(('/2.0/workspaces', list_teams)) - handler = client.handler_for_user(user_model('caboose')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'caboose' - - handler = client.handler_for_user(user_model('donut')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model is None - - # reverse it, just to be safe - authenticator.allowed_teams = ['red'] + return client - handler = client.handler_for_user(user_model('caboose')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model is None - handler = client.handler_for_user(user_model('donut')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'donut' +def user_model(username): + """ + Return a user model. + When passed to handler_for_user, it will populate + auth_model["auth_state"][authenticator.user_auth_state_key] + """ + return { + "username": username, + } -async def test_deprecated_config(caplog): - cfg = Config() - cfg.BitbucketOAuthenticator.team_whitelist = ['red'] - cfg.BitbucketOAuthenticator.whitelist = {"blue"} - log = logging.getLogger("testlog") - authenticator = BitbucketOAuthenticator(config=cfg, log=log) - assert ( - log.name, - logging.WARNING, - 'BitbucketOAuthenticator.team_whitelist is deprecated in BitbucketOAuthenticator 0.12.0, use ' - 'BitbucketOAuthenticator.allowed_teams instead', - ) in caplog.record_tuples +@mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + ("06", {"allowed_teams": {"group1"}}, True, None), + ("07", {"allowed_teams": {"test-user-not-in-group"}}, False, None), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "admin_users": {"user1"}, + "allowed_users": {"not-test-user"}, + }, + True, + True, + ), + ], +) +async def test_bitbucket( + bitbucket_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.BitbucketOAuthenticator = Config(class_config) + c.BitbucketOAuthenticator.username_claim = "username" + authenticator = BitbucketOAuthenticator(config=c) + + handled_user_model = user_model("user1") + handler = bitbucket_client.handler_for_user(handled_user_model) + auth_model = await authenticator.get_authenticated_user(handler, None) - assert authenticator.allowed_teams == {"red"} - assert authenticator.allowed_users == {"blue"} + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "access_token" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert user_info == handled_user_model + assert auth_model["name"] == user_info[authenticator.username_claim] + else: + assert auth_model == None + + +@mark.parametrize( + "test_variation_id,class_config,expect_config,expect_loglevel,expect_message", + [ + ( + "whitelist", + {"whitelist": {"dummy"}}, + {"allowed_users": {"dummy"}}, + logging.WARNING, + "BitbucketOAuthenticator.whitelist is deprecated in JupyterHub 1.2, use BitbucketOAuthenticator.allowed_users instead", + ), + ( + "team_whitelist", + {"team_whitelist": {"dummy"}}, + {"allowed_teams": {"dummy"}}, + logging.WARNING, + "BitbucketOAuthenticator.team_whitelist is deprecated in BitbucketOAuthenticator 0.12.0, use BitbucketOAuthenticator.allowed_teams instead", + ), + ], +) +async def test_deprecated_config( + caplog, + test_variation_id, + class_config, + expect_config, + expect_loglevel, + expect_message, +): + """ + Tests that a warning is emitted when using a deprecated config and that + configuring the old config ends up configuring the new config. + """ + print(f"Running test variation id {test_variation_id}") + c = Config() + c.BitbucketOAuthenticator = Config(class_config) + + test_logger = logging.getLogger('testlog') + if expect_loglevel == logging.ERROR: + with raises(ValueError, match=expect_message): + BitbucketOAuthenticator(config=c, log=test_logger) + else: + authenticator = BitbucketOAuthenticator(config=c, log=test_logger) + for key, value in expect_config.items(): + assert getattr(authenticator, key) == value + + captured_log_tuples = caplog.record_tuples + print(captured_log_tuples) + + expected_log_tuple = (test_logger.name, expect_loglevel, expect_message) + assert expected_log_tuple in captured_log_tuples diff --git a/oauthenticator/tests/test_cilogon.py b/oauthenticator/tests/test_cilogon.py index bb7b2f8a..83b66f50 100644 --- a/oauthenticator/tests/test_cilogon.py +++ b/oauthenticator/tests/test_cilogon.py @@ -2,7 +2,7 @@ import logging from jsonschema.exceptions import ValidationError -from pytest import fixture, raises +from pytest import fixture, mark, raises from tornado.web import HTTPError from traitlets.config import Config from traitlets.traitlets import TraitError @@ -11,18 +11,6 @@ from .mocks import setup_oauth_mock -def user_model(username): - """Return a user model""" - return { - 'eppn': username + '@serenity.space', - } - - -def alternative_user_model(username, claimname, **kwargs): - """Return a user model with alternate claim name""" - return {claimname: username, **kwargs} - - @fixture def cilogon_client(client): setup_oauth_mock( @@ -35,167 +23,237 @@ def cilogon_client(client): return client -async def test_cilogon(cilogon_client): - authenticator = CILogonOAuthenticator() - handler = cilogon_client.handler_for_user(user_model('wash')) - auth_model = await authenticator.get_authenticated_user(handler, None) - print(json.dumps(auth_model, sort_keys=True, indent=4)) - assert auth_model['name'] == 'wash@serenity.space' - auth_state = auth_model['auth_state'] - assert 'access_token' in auth_state - assert 'token_response' in auth_state - assert auth_state["cilogon_user"] == user_model('wash') - +def user_model(username, username_claim, **kwargs): + """Return a user model with alternate claim name""" + return { + username_claim: username, + "idp": "https://some-idp.com/login/oauth/authorize", + **kwargs, + } -async def test_cilogon_alternate_claim(cilogon_client): - authenticator = CILogonOAuthenticator(username_claim='uid') - handler = cilogon_client.handler_for_user( - alternative_user_model('jtkirk@ufp.gov', 'uid') - ) - auth_model = await authenticator.get_authenticated_user(handler, None) - print(json.dumps(auth_model, sort_keys=True, indent=4)) - assert auth_model['name'] == 'jtkirk@ufp.gov' - auth_state = auth_model['auth_state'] - assert 'access_token' in auth_state - assert 'token_response' in auth_state - assert auth_state["cilogon_user"] == alternative_user_model('jtkirk@ufp.gov', 'uid') +@mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "admin_users": {"user1"}, + "allowed_users": {"not-test-user"}, + }, + True, + True, + ), + ], +) +async def test_cilogon( + cilogon_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.CILogonOAuthenticator = Config(class_config) + c.CILogonOAuthenticator.allowed_idps = { + "https://some-idp.com/login/oauth/authorize": { + "username_derivation": { + "username_claim": "name", + }, + }, + } + authenticator = CILogonOAuthenticator(config=c) -async def test_cilogon_additional_claim(cilogon_client): - authenticator = CILogonOAuthenticator(additional_username_claims=['uid']) - handler = cilogon_client.handler_for_user( - alternative_user_model('jtkirk@ufp.gov', 'uid') - ) + handled_user_model = user_model("user1", "name") + handler = cilogon_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - print(json.dumps(auth_model, sort_keys=True, indent=4)) - assert auth_model['name'] == 'jtkirk@ufp.gov' - auth_state = auth_model['auth_state'] - assert 'access_token' in auth_state - assert 'token_response' in auth_state - assert auth_state["cilogon_user"] == alternative_user_model('jtkirk@ufp.gov', 'uid') - - -async def test_cilogon_missing_alternate_claim(cilogon_client): - authenticator = CILogonOAuthenticator() - handler = cilogon_client.handler_for_user( - alternative_user_model('jtkirk@ufp.gov', 'uid') - ) - with raises(HTTPError): - await authenticator.get_authenticated_user(handler, None) - - -async def test_deprecated_config(caplog): - cfg = Config() - cfg.CILogonOAuthenticator.idp_whitelist = ['pink'] - - log = logging.getLogger('testlog') - with raises( - ValueError, - match='CILogonOAuthenticator.idp_whitelist is deprecated in CILogonOAuthenticator 0.12.0, use ' - 'CILogonOAuthenticator.allowed_idps instead', - ): - CILogonOAuthenticator(config=cfg, log=log) - log_msgs = caplog.record_tuples - print(log_msgs) - expected_deprecation_error = ( - log.name, - logging.ERROR, - 'CILogonOAuthenticator.idp_whitelist is deprecated in CILogonOAuthenticator 0.12.0, use ' - 'CILogonOAuthenticator.allowed_idps instead', - ) - - assert expected_deprecation_error in log_msgs - - -async def test_allowed_idps_wrong_type(caplog): - # Test alllowed_idps is a dict - cfg = Config() - cfg.CILogonOAuthenticator.allowed_idps = ['pink'] + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "access_token" in auth_state + assert "token_response" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert user_info == handled_user_model + assert auth_model["name"] == user_info["name"] + else: + assert auth_model == None + + +@mark.parametrize( + "test_variation_id,class_config,expect_config,expect_loglevel,expect_message", + [ + ( + "idp_whitelist", + {"idp_whitelist": ["dummy"]}, + {}, + logging.ERROR, + "CILogonOAuthenticator.idp_whitelist is deprecated in CILogonOAuthenticator 0.12.0, use CILogonOAuthenticator.allowed_idps instead", + ), + ( + "idp", + {"idp": "dummy"}, + {}, + logging.ERROR, + "CILogonOAuthenticator.idp is deprecated in CILogonOAuthenticator 15.0.0, use CILogonOAuthenticator.shown_idps instead", + ), + ( + "strip_idp_domain", + {"strip_idp_domain": True}, + {}, + logging.ERROR, + "CILogonOAuthenticator.strip_idp_domain is deprecated in CILogonOAuthenticator 15.0.0, use CILogonOAuthenticator.allowed_idps instead", + ), + ( + "shown_idps", + {"shown_idps": ["dummy"]}, + {}, + logging.ERROR, + "CILogonOAuthenticator.shown_idps is deprecated in CILogonOAuthenticator 16.0.0, use CILogonOAuthenticator.allowed_idps instead", + ), + ( + "username_claim", + {"username_claim": "dummy"}, + {}, + logging.ERROR, + "CILogonOAuthenticator.username_claim is deprecated in CILogonOAuthenticator 16.0.0, use CILogonOAuthenticator.allowed_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", + ), + ], +) +async def test_deprecated_config( + caplog, + test_variation_id, + class_config, + expect_config, + expect_loglevel, + expect_message, +): + """ + Tests that a warning is emitted when using a deprecated config and that + configuring the old config ends up configuring the new config. + """ + print(f"Running test variation id {test_variation_id}") + c = Config() + c.CILogonOAuthenticator = Config(class_config) + + test_logger = logging.getLogger('testlog') + if expect_loglevel == logging.ERROR: + with raises(ValueError, match=expect_message): + CILogonOAuthenticator(config=c, log=test_logger) + else: + authenticator = CILogonOAuthenticator(config=c, log=test_logger) + for key, value in expect_config.items(): + assert getattr(authenticator, key) == value + + captured_log_tuples = caplog.record_tuples + print(captured_log_tuples) + + expected_log_tuple = (test_logger.name, expect_loglevel, expect_message) + assert expected_log_tuple in captured_log_tuples + + +async def test_config_allowed_idps_wrong_type(caplog): + """ + Test alllowed_idps is a dict + """ + c = Config() + c.CILogonOAuthenticator.allowed_idps = ['pink'] with raises(TraitError): - CILogonOAuthenticator(config=cfg) + CILogonOAuthenticator(config=c) -async def test_allowed_idps_required_username_derivation(caplog): +async def test_config_allowed_idps_required_username_derivation(caplog): # Test username_derivation is a required field of allowed_idps - cfg = Config() - cfg.CILogonOAuthenticator.allowed_idps = { - 'https://github.com/login/oauth/authorize': {} + c = Config() + c.CILogonOAuthenticator.allowed_idps = { + 'https://github.com/login/oauth/authorize': {}, } with raises(ValidationError, match="'username_derivation' is a required property"): - CILogonOAuthenticator(config=cfg) + CILogonOAuthenticator(config=c) -async def test_allowed_idps_invalid_entity_id(caplog): - # Test allowed_idps keys cannot be domains, but only valid CILogon entity ids, - # i.e. only fully formed URLs - cfg = Config() - cfg.CILogonOAuthenticator.allowed_idps = { +async def test_config_allowed_idps_invalid_entity_id(caplog): + """ + Test allowed_idps keys cannot be domains, but only valid CILogon entity ids, + i.e. only fully formed URLs + """ + c = Config() + c.CILogonOAuthenticator.allowed_idps = { 'uni.edu': { 'username_derivation': { 'username_claim': 'email', 'action': 'strip_idp_domain', 'domain': 'uni.edu', - } - } + }, + }, } log = logging.getLogger('testlog') with raises(ValueError): - CILogonOAuthenticator(config=cfg, log=log) + CILogonOAuthenticator(config=c, log=log) log_msgs = caplog.record_tuples - expected_deprecation_error = ( log.name, logging.ERROR, "Trying to allow an auth provider: uni.edu, that doesn't look like a valid CILogon EntityID.", ) - assert expected_deprecation_error in log_msgs -async def test_allowed_idps_invalid_config_option(caplog): - cfg = Config() - # Test config option not recognized - cfg.CILogonOAuthenticator.allowed_idps = { - 'https://github.com/login/oauth/authorize': 'invalid' - } - - with raises(ValidationError, match="'invalid' is not of type 'object'"): - CILogonOAuthenticator(config=cfg) - - -async def test_allowed_idps_invalid_config_type(caplog): - cfg = Config() - # Test username_derivation not dict - cfg.CILogonOAuthenticator.allowed_idps = { - 'https://github.com/login/oauth/authorize': 'username_derivation' +async def test_config_allowed_idps_invalid_type(caplog): + c = Config() + c.CILogonOAuthenticator.allowed_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) - with raises(ValidationError, match="'username_derivation' is not of type 'object'"): - CILogonOAuthenticator(config=cfg) - -async def test_allowed_idps_invalid_config_username_derivation_options(caplog): - cfg = Config() - # Test username_derivation not dict - cfg.CILogonOAuthenticator.allowed_idps = { +async def test_config_allowed_idps_unrecognized_options(caplog): + c = Config() + c.CILogonOAuthenticator.allowed_idps = { 'https://github.com/login/oauth/authorize': { 'username_derivation': {'a': 1, 'b': 2} } } - with raises(ValidationError, match='Additional properties are not allowed'): - CILogonOAuthenticator(config=cfg) + CILogonOAuthenticator(config=c) -async def test_allowed_idps_invalid_config_username_domain_stripping(caplog): - cfg = Config() - # Test username_derivation not dict - cfg.CILogonOAuthenticator.allowed_idps = { +async def test_config_allowed_idps_domain_required(caplog): + c = Config() + c.CILogonOAuthenticator.allowed_idps = { 'https://github.com/login/oauth/authorize': { 'username_derivation': { 'username_claim': 'email', @@ -203,15 +261,13 @@ async def test_allowed_idps_invalid_config_username_domain_stripping(caplog): } } } - with raises(ValidationError, match="'domain' is a required property"): - CILogonOAuthenticator(config=cfg) + CILogonOAuthenticator(config=c) -async def test_allowed_idps_invalid_config_username_prefix(caplog): - cfg = Config() - # Test username_derivation not dict - cfg.CILogonOAuthenticator.allowed_idps = { +async def test_config_allowed_idps_prefix_required(caplog): + c = Config() + c.CILogonOAuthenticator.allowed_idps = { 'https://github.com/login/oauth/authorize': { 'username_derivation': { 'username_claim': 'email', @@ -219,14 +275,16 @@ async def test_allowed_idps_invalid_config_username_prefix(caplog): } } } - with raises(ValidationError, match="'prefix' is a required property"): - CILogonOAuthenticator(config=cfg) + CILogonOAuthenticator(config=c) -async def test_cilogon_scopes(): - cfg = Config() - cfg.CILogonOAuthenticator.allowed_idps = { +async def test_config_scopes_validation(): + """ + Test that required scopes are appended if not configured. + """ + c = Config() + c.CILogonOAuthenticator.allowed_idps = { 'https://some-idp.com/login/oauth/authorize': { 'username_derivation': { 'username_claim': 'email', @@ -235,81 +293,89 @@ async def test_cilogon_scopes(): } } } - cfg.CILogonOAuthenticator.scope = ['email'] + c.CILogonOAuthenticator.scope = ['email'] + authenticator = CILogonOAuthenticator(config=c) - authenticator = CILogonOAuthenticator(config=cfg) expected_scopes = ['email', 'openid', 'org.cilogon.userinfo'] - assert authenticator.scope == expected_scopes -async def test_strip_and_prefix_username(cilogon_client): - cfg = Config() - cfg.CILogonOAuthenticator.allowed_idps = { - 'https://some-idp.com/login/oauth/authorize': { +async def test_allowed_idps_username_derivation_actions(cilogon_client): + c = Config() + c.CILogonOAuthenticator.allow_all = True + c.CILogonOAuthenticator.allowed_idps = { + 'https://strip-idp-domain.example.com/login/oauth/authorize': { 'username_derivation': { 'username_claim': 'email', 'action': 'strip_idp_domain', - 'domain': 'uni.edu', - } + 'domain': 'domain-to-strip.edu', + }, }, - 'https://another-idp.com/login/oauth/authorize': { + 'https://prefix.example.com/login/oauth/authorize': { 'username_derivation': { 'username_claim': 'nickname', 'action': 'prefix', 'prefix': 'idp', + }, + }, + 'https://no-action.example.com/login/oauth/authorize': { + 'username_derivation': { + 'username_claim': 'nickname', } }, } + authenticator = CILogonOAuthenticator(config=c) - authenticator = CILogonOAuthenticator(config=cfg) - - # Test stripping domain + # Test strip_idp_domain action, with domain to strip in username handler = cilogon_client.handler_for_user( - alternative_user_model( - 'jtkirk@uni.edu', 'email', idp='https://some-idp.com/login/oauth/authorize' + user_model( + 'jtkirk@domain-to-strip.edu', + 'email', + idp='https://strip-idp-domain.example.com/login/oauth/authorize', ) ) auth_model = await authenticator.get_authenticated_user(handler, None) print(json.dumps(auth_model, sort_keys=True, indent=4)) assert auth_model['name'] == 'jtkirk' - # Test appending prefixes + # Test strip_idp_domain action, without domain to strip in username handler = cilogon_client.handler_for_user( - alternative_user_model( - 'jtkirk', 'nickname', idp='https://another-idp.com/login/oauth/authorize' + user_model( + 'jtkirk@not-domain-to-strip.edu', + 'email', + idp='https://strip-idp-domain.example.com/login/oauth/authorize', ) ) auth_model = await authenticator.get_authenticated_user(handler, None) print(json.dumps(auth_model, sort_keys=True, indent=4)) - assert auth_model['name'] == 'idp:jtkirk' - + assert auth_model['name'] == 'jtkirk@not-domain-to-strip.edu' -async def test_no_action_specified(cilogon_client): - cfg = Config() - cfg.CILogonOAuthenticator.allowed_idps = { - 'https://some-idp.com/login/oauth/authorize': { - 'username_derivation': { - 'username_claim': 'email', - } - }, - } - - authenticator = CILogonOAuthenticator(config=cfg) + # Test prefix action + handler = cilogon_client.handler_for_user( + user_model( + 'jtkirk', 'nickname', idp='https://prefix.example.com/login/oauth/authorize' + ) + ) + auth_model = await authenticator.get_authenticated_user(handler, None) + print(json.dumps(auth_model, sort_keys=True, indent=4)) + assert auth_model['name'] == 'idp:jtkirk' - # Test stripping domain + # Test no action handler = cilogon_client.handler_for_user( - alternative_user_model( - 'jtkirk@uni.edu', 'email', idp='https://some-idp.com/login/oauth/authorize' + user_model( + 'jtkirk', + 'nickname', + idp='https://no-action.example.com/login/oauth/authorize', ) ) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'jtkirk@uni.edu' + print(json.dumps(auth_model, sort_keys=True, indent=4)) + assert auth_model['name'] == 'jtkirk' async def test_not_allowed_domains_and_stripping(cilogon_client): - cfg = Config() - cfg.CILogonOAuthenticator.allowed_idps = { + c = Config() + c.CILogonOAuthenticator.allowed_idps = { 'https://some-idp.com/login/oauth/authorize': { 'username_derivation': { 'username_claim': 'email', @@ -320,11 +386,11 @@ async def test_not_allowed_domains_and_stripping(cilogon_client): }, } - authenticator = CILogonOAuthenticator(config=cfg) + authenticator = CILogonOAuthenticator(config=c) # Test stripping domain not allowed handler = cilogon_client.handler_for_user( - alternative_user_model( + user_model( 'jtkirk@uni.edu', 'email', idp='https://some-idp.com/login/oauth/authorize' ) ) @@ -335,8 +401,8 @@ async def test_not_allowed_domains_and_stripping(cilogon_client): async def test_allowed_domains_and_stripping(cilogon_client): - cfg = Config() - cfg.CILogonOAuthenticator.allowed_idps = { + c = Config() + c.CILogonOAuthenticator.allowed_idps = { 'https://some-idp.com/login/oauth/authorize': { 'username_derivation': { 'username_claim': 'email', @@ -347,23 +413,21 @@ async def test_allowed_domains_and_stripping(cilogon_client): }, } - authenticator = CILogonOAuthenticator(config=cfg) + authenticator = CILogonOAuthenticator(config=c) # Test stripping allowed domain handler = cilogon_client.handler_for_user( - alternative_user_model( + user_model( 'jtkirk@pink.org', 'email', idp='https://some-idp.com/login/oauth/authorize' ) ) - - # The domain to be stripped is allowed, so it should be stripped auth_model = await authenticator.get_authenticated_user(handler, None) assert auth_model['name'] == 'jtkirk' async def test_allowed_domains_no_stripping(cilogon_client): - cfg = Config() - cfg.CILogonOAuthenticator.allowed_idps = { + c = Config() + c.CILogonOAuthenticator.allowed_idps = { 'https://some-idp.com/login/oauth/authorize': { 'username_derivation': { 'username_claim': 'email', @@ -372,24 +436,22 @@ async def test_allowed_domains_no_stripping(cilogon_client): }, } - authenticator = CILogonOAuthenticator(config=cfg) + authenticator = CILogonOAuthenticator(config=c) - # Test domain not allowed + # Test login with user not part of allowed_domains handler = cilogon_client.handler_for_user( - alternative_user_model( + user_model( 'jtkirk@uni.edu', 'email', idp='https://some-idp.com/login/oauth/authorize' ) ) - with raises(HTTPError): auth_model = await authenticator.get_authenticated_user(handler, None) - # Test allowed domain login + # Test login with part of allowed_domains handler = cilogon_client.handler_for_user( - alternative_user_model( + user_model( 'jtkirk@pink.org', 'email', idp='https://some-idp.com/login/oauth/authorize' ) ) - auth_model = await authenticator.get_authenticated_user(handler, None) assert auth_model['name'] == 'jtkirk@pink.org' diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py index 575a6e22..82290ea4 100644 --- a/oauthenticator/tests/test_generic.py +++ b/oauthenticator/tests/test_generic.py @@ -1,6 +1,7 @@ from functools import partial -from pytest import fixture +from pytest import fixture, mark +from traitlets.config import Config from ..generic import GenericOAuthenticator from .mocks import setup_oauth_mock @@ -8,24 +9,12 @@ def user_model(username, **kwargs): """Return a user model""" - user = { - 'username': username, - 'scope': 'basic', - } - user.update(kwargs) - return user - - -def _get_authenticator(**kwargs): - return GenericOAuthenticator( - token_url='https://generic.horse/oauth/access_token', - userdata_url='https://generic.horse/oauth/userinfo', + return { + "username": username, + "scope": "basic", + "groups": ["group1"], **kwargs, - ) - - -def get_simple_handler(generic_client): - return generic_client.handler_for_user(user_model('wash')) + } @fixture @@ -39,193 +28,213 @@ def generic_client(client): return client +def _get_authenticator(**kwargs): + return GenericOAuthenticator( + token_url='https://generic.horse/oauth/access_token', + userdata_url='https://generic.horse/oauth/userinfo', + **kwargs, + ) + + @fixture -def get_authenticator(generic_client, **kwargs): +def get_authenticator(generic_client): + """ + http_client can't be configured, only passed as argument to the constructor. + """ return partial(_get_authenticator, http_client=generic_client) -async def test_generic(get_authenticator, generic_client): - authenticator = get_authenticator() - - handler = get_simple_handler(generic_client) +@mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + ("06", {"allowed_groups": {"group1"}}, True, None), + ("07", {"allowed_groups": {"test-user-not-in-group"}}, False, None), + ("08", {"admin_groups": {"group1"}}, True, True), + ("09", {"admin_groups": {"test-user-not-in-group"}}, False, False), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "allowed_users": {"not-test-user"}, + "admin_users": {"user1"}, + }, + True, + True, + ), + ( + "12", + { + "allowed_groups": {"group1"}, + "admin_groups": {"group1"}, + }, + True, + True, + ), + ( + "13", + { + "allowed_groups": {"group1"}, + "admin_groups": {"test-user-not-in-group"}, + }, + True, + False, + ), + ( + "14", + { + "allowed_groups": {"test-user-not-in-group"}, + "admin_groups": {"group1"}, + }, + True, + True, + ), + ( + "15", + { + "allowed_groups": {"test-user-not-in-group"}, + "admin_groups": {"test-user-not-in-group"}, + }, + False, + False, + ), + ( + "16", + { + "admin_users": {"user1"}, + "admin_groups": {"group1"}, + }, + True, + True, + ), + ( + "17", + { + "admin_users": {"user1"}, + "admin_groups": {"test-user-not-in-group"}, + }, + True, + True, + ), + ( + "18", + { + "admin_users": {"not-test-user"}, + "admin_groups": {"group1"}, + }, + True, + True, + ), + ( + "19", + { + "admin_users": {"not-test-user"}, + "admin_groups": {"test-user-not-in-group"}, + }, + False, + False, + ), + ], +) +async def test_generic_asd( + get_authenticator, + generic_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.GenericOAuthenticator = Config(class_config) + c.GenericOAuthenticator.username_claim = "username" + authenticator = get_authenticator(config=c) + + handled_user_model = user_model("user1") + handler = generic_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert sorted(auth_model) == ['admin', 'auth_state', 'name'] - assert auth_model['name'] == 'wash' - auth_state = auth_model['auth_state'] - assert 'access_token' in auth_state - assert 'oauth_user' in auth_state - assert 'refresh_token' in auth_state - assert 'scope' in auth_state + + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "access_token" in auth_state + assert "oauth_user" in auth_state + assert "refresh_token" in auth_state + assert "scope" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert auth_model["name"] == user_info[authenticator.username_claim] + else: + assert auth_model == None async def test_generic_data(get_authenticator, generic_client): + c = Config() + c.GenericOAuthenticator.allow_all = True authenticator = get_authenticator() - handler = get_simple_handler(generic_client) - data = {'testing': 'data'} + handled_user_model = user_model("user1") + handler = generic_client.handler_for_user(handled_user_model) + data = {"testing": "data"} auth_model = await authenticator.authenticate(handler, data) - assert sorted(auth_model) == ['admin', 'auth_state', 'name'] - assert auth_model['name'] == 'wash' - - -async def test_generic_callable_username_key(get_authenticator, generic_client): - authenticator = get_authenticator(username_key=lambda r: r['alternate_username']) - handler = generic_client.handler_for_user( - user_model('wash', alternate_username='zoe') - ) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'zoe' + assert auth_model -async def test_generic_callable_groups_claim_key_with_allowed_groups( - get_authenticator, generic_client -): - authenticator = get_authenticator( - scope=['openid', 'profile', 'roles'], - claim_groups_key=lambda r: r.get('policies').get('roles'), - allowed_groups=['super_user'], - ) - handler = generic_client.handler_for_user( - user_model('wash', alternate_username='zoe', policies={'roles': ['super_user']}) - ) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' +async def test_generic_callable_username_key(get_authenticator, generic_client): + c = Config() + c.GenericOAuthenticator.allow_all = True + c.GenericOAuthenticator.username_key = lambda r: r["alternate_username"] + authenticator = get_authenticator(config=c) -async def test_generic_groups_claim_key_with_allowed_groups( - get_authenticator, generic_client -): - authenticator = get_authenticator( - scope=['openid', 'profile', 'roles'], - claim_groups_key='groups', - allowed_groups=['super_user'], - ) - handler = generic_client.handler_for_user( - user_model('wash', alternate_username='zoe', groups=['super_user']) - ) + handled_user_model = user_model("user1", alternate_username="zoe") + handler = generic_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' + assert auth_model["name"] == "zoe" -async def test_generic_groups_claim_key_nested_strings( - get_authenticator, generic_client -): - authenticator = get_authenticator( - scope=['openid', 'profile', 'roles'], - claim_groups_key='permissions.groups', - allowed_groups=['super_user'], - ) - handler = generic_client.handler_for_user( - user_model( - 'wash', alternate_username='zoe', permissions={"groups": ['super_user']} - ) - ) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' +async def test_generic_claim_groups_key_callable(get_authenticator, generic_client): + c = Config() + c.GenericOAuthenticator.claim_groups_key = lambda r: r["policies"]["roles"] + c.GenericOAuthenticator.allowed_groups = ["super_user"] + authenticator = get_authenticator(config=c) -async def test_generic_groups_claim_key_nested_strings_nonexistant_key( - get_authenticator, generic_client -): - authenticator = get_authenticator( - scope=['openid', 'profile', 'roles'], - claim_groups_key='permissions.groups', - allowed_groups=['super_user'], - ) - handler = generic_client.handler_for_user( - user_model('wash', alternate_username='zoe') - ) + handled_user_model = user_model("user1", policies={"roles": ["super_user"]}) + handler = generic_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model is None + assert auth_model -async def test_generic_groups_claim_key_with_allowed_groups_unauthorized( - get_authenticator, generic_client -): - authenticator = get_authenticator( - scope=['openid', 'profile', 'roles'], - claim_groups_key='groups', - allowed_groups=['user'], - ) - handler = generic_client.handler_for_user( - user_model('wash', alternate_username='zoe', groups=['public']) - ) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model is None - -async def test_generic_groups_claim_key_with_allowed_groups_and_admin_groups( +async def test_generic_claim_groups_key_nested_strings( get_authenticator, generic_client ): - authenticator = get_authenticator( - scope=['openid', 'profile', 'roles'], - claim_groups_key='groups', - allowed_groups=['user'], - admin_groups=['administrator'], - ) - handler = generic_client.handler_for_user( - user_model('wash', alternate_username='zoe', groups=['user', 'administrator']) - ) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' - assert auth_model['admin'] is True + c = Config() + c.GenericOAuthenticator.claim_groups_key = "permissions.groups" + c.GenericOAuthenticator.admin_groups = ["super_user"] + authenticator = get_authenticator(config=c) - -async def test_generic_groups_claim_key_with_allowed_groups_and_admin_groups_not_admin( - get_authenticator, generic_client -): - authenticator = get_authenticator( - scope=['openid', 'profile', 'roles'], - claim_groups_key='groups', - allowed_groups=['user'], - admin_groups=['administrator'], - ) - handler = generic_client.handler_for_user( - user_model('wash', alternate_username='zoe', groups=['user']) - ) + handled_user_model = user_model("user1", permissions={"groups": ["super_user"]}) + handler = generic_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' - assert auth_model['admin'] is False - - -async def test_generic_groups_claim_key_with_allowed_groups_and_no_admin_groups_but_admin_users( - get_authenticator, generic_client -): - authenticator = get_authenticator( - scope=['openid', 'profile', 'roles'], - claim_groups_key='groups', - allowed_groups=['user'], - admin_groups=[], - admin_users=['wash'], - ) - handler = generic_client.handler_for_user( - user_model('wash', alternate_username='zoe', groups=['user']) - ) - - # Assert that the authenticated user is actually an admin due to being listed in `admin_users` - # Even though admin_groups is empty - auth_model = await authenticator.get_authenticated_user(handler, data=None) - assert auth_model['name'] == 'wash' - assert auth_model['admin'] is True - -async def test_generic_callable_groups_claim_key_with_allowed_groups_and_admin_groups( - get_authenticator, generic_client -): - authenticator = get_authenticator( - username_key=lambda r: r['alternate_username'], - scope=['openid', 'profile', 'roles'], - claim_groups_key=lambda r: r.get('policies').get('roles'), - allowed_groups=['user', 'public'], - admin_groups=['administrator'], - ) - handler = generic_client.handler_for_user( - user_model( - 'wash', - alternate_username='zoe', - policies={'roles': ['user', 'administrator']}, - ) - ) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'zoe' - assert auth_model['admin'] is True + assert auth_model + assert auth_model["admin"] diff --git a/oauthenticator/tests/test_github.py b/oauthenticator/tests/test_github.py index 8fd2bbc7..55b91f48 100644 --- a/oauthenticator/tests/test_github.py +++ b/oauthenticator/tests/test_github.py @@ -5,7 +5,7 @@ from io import BytesIO from urllib.parse import parse_qs, urlparse -from pytest import fixture +from pytest import fixture, mark, raises from tornado.httpclient import HTTPResponse from tornado.httputil import HTTPHeaders from traitlets.config import Config @@ -36,20 +36,66 @@ def github_client(client): return client -async def test_github(github_client): - authenticator = GitHubOAuthenticator() - handler = github_client.handler_for_user(user_model('wash')) +@mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "admin_users": {"user1"}, + "allowed_users": {"not-test-user"}, + }, + True, + True, + ), + ], +) +async def test_github( + github_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.GitHubOAuthenticator = Config(class_config) + c.GitHubOAuthenticator.username_claim = "login" + authenticator = GitHubOAuthenticator(config=c) + + handled_user_model = user_model("user1") + handler = github_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' - auth_state = auth_model['auth_state'] - assert 'access_token' in auth_state - assert 'github_user' in auth_state - assert auth_state["github_user"] == { - 'email': 'dinosaurs@space', - 'id': 5, - 'login': auth_model['name'], - 'name': 'Hoban Washburn', - } + + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "access_token" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert user_info == handled_user_model + assert auth_model["name"] == user_info[authenticator.username_claim] + else: + assert auth_model == None def make_link_header(urlinfo, page): @@ -59,17 +105,18 @@ def make_link_header(urlinfo, page): async def test_allowed_org_membership(github_client): - client = github_client authenticator = GitHubOAuthenticator() ## Mock Github API - orgs = { - 'red': ['grif', 'simmons', 'donut', 'sarge', 'lopez'], - 'blue': ['tucker', 'caboose', 'burns', 'sheila', 'texas'], + allowed_org_members = { + "org1": ["user1"], + } + allowed_org_team_members = { + "org1": { + "team1": ["user1"], + }, } - - org_teams = {'blue': {'alpha': ['tucker', 'caboose', 'burns']}} member_regex = re.compile(r'/orgs/(.*)/members') @@ -77,11 +124,11 @@ def org_members(paginate, request): urlinfo = urlparse(request.url) org = member_regex.match(urlinfo.path).group(1) - if org not in orgs: + if org not in allowed_org_members: return HTTPResponse(request, 404) if not paginate: - return [user_model(m) for m in orgs[org]] + return [user_model(m) for m in allowed_org_members[org]] else: page = parse_qs(urlinfo.query).get('page', ['1']) page = int(page[0]) @@ -90,16 +137,16 @@ def org_members(paginate, request): ) def org_members_paginated(org, page, urlinfo, response): - if page < len(orgs[org]): + if page < len(allowed_org_members[org]): headers = make_link_header(urlinfo, page + 1) - elif page == len(orgs[org]): + elif page == len(allowed_org_members[org]): headers = {} else: return response(400) headers.update({'Content-Type': 'application/json'}) - ret = [user_model(orgs[org][page - 1])] + ret = [user_model(allowed_org_members[org][page - 1])] return response( 200, @@ -115,10 +162,10 @@ def org_membership(request): org = urlmatch.group(1) username = urlmatch.group(2) print(f"Request org = {org}, username = {username}") - if org not in orgs: + if org not in allowed_org_members: print(f"Org not found: org = {org}") return HTTPResponse(request, 404) - if username not in orgs[org]: + if username not in allowed_org_members[org]: print(f"Member not found: org = {org}, username = {username}") return HTTPResponse(request, 404) return HTTPResponse(request, 204) @@ -132,13 +179,13 @@ def team_membership(request): team = urlmatch.group(2) username = urlmatch.group(3) print(f"Request org = {org}, team = {team} username = {username}") - if org not in orgs: + if org not in allowed_org_members: print(f"Org not found: org = {org}") return HTTPResponse(request, 404) - if team not in org_teams[org]: + if team not in allowed_org_team_members[org]: print(f"Team not found in org: team = {team}, org = {org}") return HTTPResponse(request, 404) - if username not in org_teams[org][team]: + if username not in allowed_org_team_members[org][team]: print( f"Member not found: org = {org}, team = {team}, username = {username}" ) @@ -147,65 +194,82 @@ def team_membership(request): ## Perform tests + client_hosts = github_client.hosts['api.github.com'] + client_hosts.append((team_membership_regex, team_membership)) + client_hosts.append((org_membership_regex, org_membership)) + + # Run tests twice, once with paginate and once without for paginate in (False, True): - client_hosts = client.hosts['api.github.com'] - client_hosts.append((team_membership_regex, team_membership)) - client_hosts.append((org_membership_regex, org_membership)) client_hosts.append((member_regex, functools.partial(org_members, paginate))) - authenticator.allowed_organizations = ['blue'] - - handler = client.handler_for_user(user_model('caboose')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'caboose' + # test org membership + authenticator.allowed_organizations = ["org1"] - handler = client.handler_for_user(user_model('donut')) + handled_user_model = user_model("user1") + handler = github_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model is None - - # reverse it, just to be safe - authenticator.allowed_organizations = ['red'] + assert auth_model - handler = client.handler_for_user(user_model('caboose')) + handled_user_model = user_model("user-not-in-org") + handler = github_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) assert auth_model is None - handler = client.handler_for_user(user_model('donut')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'donut' - - # test team membership - authenticator.allowed_organizations = ['blue:alpha', 'red'] + # test org team membership + authenticator.allowed_organizations = ["org1:team1"] - handler = client.handler_for_user(user_model('tucker')) + handled_user_model = user_model("user1") + handler = github_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'tucker' + assert auth_model - handler = client.handler_for_user(user_model('grif')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'grif' - - handler = client.handler_for_user(user_model('texas')) + handled_user_model = user_model("user-not-in-org-team") + handler = github_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) assert auth_model is None client_hosts.pop() - client_hosts.pop() - - -async def test_deprecated_config(caplog): - cfg = Config() - cfg.GitHubOAuthenticator.github_organization_whitelist = ["jupy"] - cfg.Authenticator.whitelist = {"user1"} - log = logging.getLogger("testlog") - authenticator = GitHubOAuthenticator(config=cfg, log=log) - assert ( - log.name, - logging.WARNING, - 'GitHubOAuthenticator.github_organization_whitelist is deprecated in GitHubOAuthenticator 0.12.0, use ' - 'GitHubOAuthenticator.allowed_organizations instead', - ) in caplog.record_tuples - assert authenticator.allowed_organizations == {"jupy"} - assert authenticator.allowed_users == {"user1"} +@mark.parametrize( + "test_variation_id,class_config,expect_config,expect_loglevel,expect_message", + [ + ( + "github_organization_whitelist", + {"github_organization_whitelist": {"dummy"}}, + {"allowed_organizations": {"dummy"}}, + logging.WARNING, + "GitHubOAuthenticator.github_organization_whitelist is deprecated in GitHubOAuthenticator 0.12.0, use GitHubOAuthenticator.allowed_organizations instead", + ), + ], +) +async def test_deprecated_config( + caplog, + test_variation_id, + class_config, + expect_config, + expect_loglevel, + expect_message, +): + """ + Tests that a warning is emitted when using a deprecated config and that + configuring the old config ends up configuring the new config. + """ + print(f"Running test variation id {test_variation_id}") + c = Config() + c.GitHubOAuthenticator = Config(class_config) + + test_logger = logging.getLogger('testlog') + if expect_loglevel == logging.ERROR: + with raises(ValueError, match=expect_message): + GitHubOAuthenticator(config=c, log=test_logger) + else: + authenticator = GitHubOAuthenticator(config=c, log=test_logger) + for key, value in expect_config.items(): + assert getattr(authenticator, key) == value + + captured_log_tuples = caplog.record_tuples + print(captured_log_tuples) + + expected_log_tuple = (test_logger.name, expect_loglevel, expect_message) + assert expected_log_tuple in captured_log_tuples diff --git a/oauthenticator/tests/test_gitlab.py b/oauthenticator/tests/test_gitlab.py index 044d5764..e9e4ae22 100644 --- a/oauthenticator/tests/test_gitlab.py +++ b/oauthenticator/tests/test_gitlab.py @@ -1,4 +1,3 @@ -import collections import functools import json import logging @@ -6,7 +5,7 @@ from io import BytesIO from urllib.parse import parse_qs, urlparse -from pytest import fixture +from pytest import fixture, mark, raises from tornado.httpclient import HTTPResponse from tornado.httputil import HTTPHeaders from traitlets.config import Config @@ -17,17 +16,19 @@ API_ENDPOINT = f"/api/v{GitLabOAuthenticator().gitlab_api_version}" -def user_model(username, id=1, is_admin=False): +id_to_username_map = {} + + +def user_model(username): """Return a user model""" - user = { - 'username': username, - 'id': id, + + # generate an id based on the username hash and remember it + id = abs(hash(username)) % (10**8) + id_to_username_map[id] = username + return { + "username": username, + "id": id, } - if is_admin: - # Some versions of the API do not return the is_admin property - # for non-admin users (See #115). - user['is_admin'] = True - return user @fixture @@ -55,16 +56,66 @@ def mock_version_response(request): client.hosts['gitlab.com'].append((regex, mock_version_response)) -async def test_gitlab(gitlab_client): - authenticator = GitLabOAuthenticator() - mock_api_version(gitlab_client, '12.3.1-ee') - handler = gitlab_client.handler_for_user(user_model('wash')) +@mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "admin_users": {"user1"}, + "allowed_users": {"not-test-user"}, + }, + True, + True, + ), + ], +) +async def test_gitlab( + gitlab_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.GitLabOAuthenticator = Config(class_config) + c.GitLabOAuthenticator.username_claim = "username" + authenticator = GitLabOAuthenticator(config=c) + + handled_user_model = user_model("user1") + handler = gitlab_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert sorted(auth_model) == ['admin', 'auth_state', 'name'] - assert auth_model['name'] == 'wash' - auth_state = auth_model['auth_state'] - assert 'access_token' in auth_state - assert 'gitlab_user' in auth_state + + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "access_token" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert user_info == handled_user_model + assert auth_model["name"] == user_info[authenticator.username_claim] + else: + assert auth_model == None def make_link_header(urlinfo, page): @@ -73,151 +124,152 @@ def make_link_header(urlinfo, page): } -async def test_allowed_groups(gitlab_client): - client = gitlab_client +@mark.parametrize( + "paginate", + [ + False, + True, + ], +) +async def test_allowed_groups(gitlab_client, paginate): authenticator = GitLabOAuthenticator() - mock_api_version(client, '12.4.0-ee') + mock_api_version(gitlab_client, '12.4.0-ee') ## set up fake Gitlab API - user_groups = collections.OrderedDict( - { - 'grif': ['red', 'yellow'], - 'simmons': ['red', 'yellow'], - 'caboose': ['blue', 'yellow'], - 'burns': ['blue', 'yellow'], - } - ) + user_groups = { + "user1": ["group0", "group1"], + } - def group_user_model(username, is_admin=False): - return user_model( - username, list(user_groups.keys()).index(username) + 1, is_admin - ) + groups_members_api_regex = re.compile( + API_ENDPOINT + r'/groups/(.*)/members/all/(.*)' + ) - member_regex = re.compile(API_ENDPOINT + r'/groups/(.*)/members/all/(.*)') + def mocked_groups_members_api(request): + """ + Is user_id a member of a group? - def is_member(request): + https://docs.gitlab.com/ee/api/members.html#get-a-member-of-a-group-or-project + is mocked solely by the HTTP response code. + """ urlinfo = urlparse(request.url) - group, uid = member_regex.match(urlinfo.path).group(1, 2) - uname = list(user_groups.keys())[int(uid) - 1] - if group in user_groups[uname]: + group, user_id = groups_members_api_regex.match(urlinfo.path).group(1, 2) + username = id_to_username_map[int(user_id)] + if group in user_groups.get(username, []): return HTTPResponse(request, 200) else: return HTTPResponse(request, 404) - def groups(paginate, request): + def mocked_groups_api(paginate, request): + """ + What groups are the user that makes a request to the /groups API part + of? + + https://docs.gitlab.com/ee/api/groups.html#list-groups is mocked by + returning a list of dictionaries like {"path": }, and only + one group per page. + """ urlinfo = urlparse(request.url) _, token = request._headers.get('Authorization').split() - user = client.access_tokens[token]['username'] + username = gitlab_client.access_tokens[token]['username'] if not paginate: - return [{'path': group} for group in user_groups[user]] + return [{'path': group} for group in user_groups[username]] else: page = parse_qs(urlinfo.query).get('page', ['1']) page = int(page[0]) - return groups_paginated( - user, page, urlinfo, functools.partial(HTTPResponse, request) + return _mocked_groups_api_paginated( + username, page, urlinfo, functools.partial(HTTPResponse, request) ) - def groups_paginated(user, page, urlinfo, response): - if page < len(user_groups[user]): + def _mocked_groups_api_paginated(username, page, urlinfo, response): + """ + Helper function for mocked_groups_api. + """ + if page < len(user_groups[username]): headers = make_link_header(urlinfo, page + 1) - elif page == len(user_groups[user]): + elif page == len(user_groups[username]): headers = {} else: return response(400) headers.update({'Content-Type': 'application/json'}) - - ret = [{'path': user_groups[user][page - 1]}] - + ret = [{'path': user_groups[username][page - 1]}] return response( 200, headers=HTTPHeaders(headers), buffer=BytesIO(json.dumps(ret).encode('utf-8')), ) - client.hosts['gitlab.com'].append((member_regex, is_member)) + gitlab_client.hosts['gitlab.com'].append( + (groups_members_api_regex, mocked_groups_members_api) + ) + gitlab_client.hosts['gitlab.com'].append( + (API_ENDPOINT + '/groups', functools.partial(mocked_groups_api, paginate)) + ) ## actual tests - for paginate in (False, True): - client.hosts['gitlab.com'].append( - (API_ENDPOINT + '/groups', functools.partial(groups, paginate)) - ) - - authenticator.allowed_gitlab_groups = ['blue'] - - handler = client.handler_for_user(group_user_model('caboose')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'caboose' - - handler = client.handler_for_user(group_user_model('burns', is_admin=True)) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'burns' - - handler = client.handler_for_user(group_user_model('grif')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model is None + authenticator.allowed_gitlab_groups = ["group1"] - handler = client.handler_for_user(group_user_model('simmons', is_admin=True)) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model is None - - # reverse it, just to be safe - authenticator.allowed_gitlab_groups = ['red'] - - handler = client.handler_for_user(group_user_model('caboose')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model is None - - handler = client.handler_for_user(group_user_model('grif')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'grif' + handled_user_model = user_model("user1") + handler = gitlab_client.handler_for_user(handled_user_model) + auth_model = await authenticator.get_authenticated_user(handler, None) + assert auth_model - client.hosts['gitlab.com'].pop() + handled_user_model = user_model("user-not-in-group") + handler = gitlab_client.handler_for_user(handled_user_model) + auth_model = await authenticator.get_authenticated_user(handler, None) + assert auth_model is None async def test_allowed_project_ids(gitlab_client): - client = gitlab_client authenticator = GitLabOAuthenticator() - mock_api_version(client, '12.4.0-pre') + mock_api_version(gitlab_client, '12.4.0-pre') + non_project_member_user_model = user_model('non-project-member') + guest_user_model = user_model('guest') + developer_user_model = user_model('developer') user_projects = { - '1231231': { - '3588673': { - 'id': 3588674, - 'name': 'john', - 'username': 'john', + '1': { + str(guest_user_model["id"]): { + 'id': guest_user_model["id"], + 'name': guest_user_model["username"], + 'username': guest_user_model["username"], 'state': 'active', 'avatar_url': 'https://secure.gravatar.com/avatar/382a6b306679b2d97b547bfff3d73242?s=80&d=identicon', - 'web_url': 'https://gitlab.com/john', + 'web_url': f'https://gitlab.com/{guest_user_model["username"]}', 'access_level': 10, # Guest - 'expires_at': '2030-02-23', + 'expires_at': '2040-02-23', }, - '3588674': { - 'id': 3588674, - 'name': 'harry', - 'username': 'harry', + str(developer_user_model["id"]): { + 'id': developer_user_model["id"], + 'name': developer_user_model["username"], + 'username': developer_user_model["username"], 'state': 'active', 'avatar_url': 'https://secure.gravatar.com/avatar/382a6b306679b2d97b547bfff3d73242?s=80&d=identicon', - 'web_url': 'https://gitlab.com/harry', + 'web_url': f'https://gitlab.com/{guest_user_model["username"]}', 'access_level': 30, # Developer - 'expires_at': '2030-02-23', + 'expires_at': '2040-02-23', }, } } - john_user_model = user_model('john', 3588673) - harry_user_model = user_model('harry', 3588674) - sheila_user_model = user_model('sheila', 3588675) - member_regex = re.compile(API_ENDPOINT + r'/projects/(.*)/members/all/(.*)') + projects_members_api_regex = re.compile( + API_ENDPOINT + r'/projects/(.*)/members/all/(.*)' + ) - def is_member(request): + def mocked_projects_members_api(request): + """ + Is user_id a member of a project? + + https://docs.gitlab.com/ee/api/members.html#get-a-member-of-a-group-or-project + is mocked by somewhat realistic response. + """ urlinfo = urlparse(request.url) - project_id, uid = member_regex.match(urlinfo.path).group(1, 2) + project_id, user_id = projects_members_api_regex.match(urlinfo.path).group(1, 2) - if user_projects.get(project_id) and user_projects.get(project_id).get(uid): - res = user_projects.get(project_id).get(uid) + if user_projects.get(project_id) and user_projects[project_id].get(user_id): + res = user_projects[project_id][user_id] return HTTPResponse( request=request, code=200, @@ -227,55 +279,79 @@ def is_member(request): else: return HTTPResponse(request=request, code=404, buffer=BytesIO(b'')) - client.hosts['gitlab.com'].append((member_regex, is_member)) + gitlab_client.hosts['gitlab.com'].append( + (projects_members_api_regex, mocked_projects_members_api) + ) - authenticator.allowed_project_ids = [1231231] + authenticator.allowed_project_ids = [1] - # Forbidden since John has guest access - handler = client.handler_for_user(john_user_model) + # Forbidden, user doesn't have access to a project in allowed_project_ids + handler = gitlab_client.handler_for_user(non_project_member_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) assert auth_model is None - # Authenticated since Harry has developer access to the project - handler = client.handler_for_user(harry_user_model) - auth_model = await authenticator.get_authenticated_user(handler, None) - name = auth_model['name'] - assert name == 'harry' - - # Forbidden since Sheila doesn't have access to the project - handler = client.handler_for_user(sheila_user_model) + # Forbidden, user only has has guest access a project in allowed_project_ids + handler = gitlab_client.handler_for_user(guest_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) assert auth_model is None - authenticator.allowed_project_ids = [123123152543] + # Authorized, user has developer access a project in allowed_project_ids + handler = gitlab_client.handler_for_user(developer_user_model) + auth_model = await authenticator.get_authenticated_user(handler, None) + assert auth_model - # Forbidden since the project does not exist. - handler = client.handler_for_user(harry_user_model) + # Forbidden, project doesn't exist + authenticator.allowed_project_ids = [0] + handler = gitlab_client.handler_for_user(developer_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) assert auth_model is None - authenticator.allowed_project_ids = [123123152543, 1231231] - - # Authenticated since Harry has developer access to one of the project in the list - handler = client.handler_for_user(harry_user_model) + # Authorized, user has developer access to one of the allowed_project_ids + authenticator.allowed_project_ids = [0, 1] + handler = gitlab_client.handler_for_user(developer_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - name = auth_model['name'] - assert name == 'harry' - - -async def test_deprecated_config(caplog): - cfg = Config() - cfg.GitLabOAuthenticator.gitlab_group_whitelist = {'red'} - cfg.GitLabOAuthenticator.whitelist = {"blue"} - - log = logging.getLogger("testlog") - authenticator = GitLabOAuthenticator(config=cfg, log=log) - assert ( - log.name, - logging.WARNING, - 'GitLabOAuthenticator.gitlab_group_whitelist is deprecated in GitLabOAuthenticator 0.12.0, use ' - 'GitLabOAuthenticator.allowed_gitlab_groups instead', - ) in caplog.record_tuples - - assert authenticator.allowed_gitlab_groups == {'red'} - assert authenticator.allowed_users == {"blue"} + assert auth_model + + +@mark.parametrize( + "test_variation_id,class_config,expect_config,expect_loglevel,expect_message", + [ + ( + "gitlab_group_whitelist", + {"gitlab_group_whitelist": {"dummy"}}, + {"allowed_gitlab_groups": {"dummy"}}, + logging.WARNING, + "GitLabOAuthenticator.gitlab_group_whitelist is deprecated in GitLabOAuthenticator 0.12.0, use GitLabOAuthenticator.allowed_gitlab_groups instead", + ), + ], +) +async def test_deprecated_config( + caplog, + test_variation_id, + class_config, + expect_config, + expect_loglevel, + expect_message, +): + """ + Tests that a warning is emitted when using a deprecated config and that + configuring the old config ends up configuring the new config. + """ + print(f"Running test variation id {test_variation_id}") + c = Config() + c.GitLabOAuthenticator = Config(class_config) + + test_logger = logging.getLogger('testlog') + if expect_loglevel == logging.ERROR: + with raises(ValueError, match=expect_message): + GitLabOAuthenticator(config=c, log=test_logger) + else: + authenticator = GitLabOAuthenticator(config=c, log=test_logger) + for key, value in expect_config.items(): + assert getattr(authenticator, key) == value + + captured_log_tuples = caplog.record_tuples + print(captured_log_tuples) + + expected_log_tuple = (test_logger.name, expect_loglevel, expect_message) + assert expected_log_tuple in captured_log_tuples diff --git a/oauthenticator/tests/test_globus.py b/oauthenticator/tests/test_globus.py index 8560e0b0..7dbd1476 100644 --- a/oauthenticator/tests/test_globus.py +++ b/oauthenticator/tests/test_globus.py @@ -3,23 +3,22 @@ from unittest.mock import Mock from urllib.parse import parse_qs -from pytest import fixture, raises +from pytest import fixture, mark, raises from tornado import web from tornado.httpclient import HTTPResponse +from traitlets.config import Config from ..globus import GlobusLogoutHandler, GlobusOAuthenticator from ..oauth2 import STATE_COOKIE_NAME from .mocks import mock_handler, setup_oauth_mock -def user_model(username, email=None): +def user_model(username, **kwargs): """Return a user model""" - userinfo = { - 'preferred_username': username, + return { + "preferred_username": username, + **kwargs, } - if email: - userinfo['email'] = email - return userinfo def revoke_token_request_handler(request): @@ -72,21 +71,11 @@ def mock_globus_token_response(): def get_groups_request_handler(request): mock_globus_groups_response = [ { - 'id': '21c6bc5d-fc12-4f60-b999-76766cd596c2', - 'my_memberships': [{'role': 'manager'}], - }, - { - 'id': '915dcd61-c842-4ea4-97c6-57396b936016', + # group's IDs should really be UUIDs, but a simpler string is used + # for consistency between tests + 'id': 'group1', 'my_memberships': [{'role': 'member'}], }, - { - 'id': 'd11abe71-5132-4c04-a4ad-50926885dc8c', - 'my_memberships': [ - { - 'role': 'member', - } - ], - }, ] assert request.method == 'GET', request.method resp = BytesIO(json.dumps(mock_globus_groups_response).encode('utf-8')) @@ -184,13 +173,144 @@ async def save_auth_state(self, state): return User() -async def test_globus(globus_client): - authenticator = GlobusOAuthenticator() - handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu')) +@mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + ("06", {"allowed_globus_groups": {"group1"}}, True, None), + ("07", {"allowed_globus_groups": {"test-user-not-in-group"}}, False, None), + ("08", {"admin_globus_groups": {"group1"}}, True, True), + ("09", {"admin_globus_groups": {"test-user-not-in-group"}}, False, False), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "allowed_users": {"not-test-user"}, + "admin_users": {"user1"}, + }, + True, + True, + ), + ( + "12", + { + "allowed_globus_groups": {"group1"}, + "admin_globus_groups": {"group1"}, + }, + True, + True, + ), + ( + "13", + { + "allowed_globus_groups": {"group1"}, + "admin_globus_groups": {"test-user-not-in-group"}, + }, + True, + False, + ), + ( + "14", + { + "allowed_globus_groups": {"test-user-not-in-group"}, + "admin_globus_groups": {"group1"}, + }, + True, + True, + ), + ( + "15", + { + "allowed_globus_groups": {"test-user-not-in-group"}, + "admin_globus_groups": {"test-user-not-in-group"}, + }, + False, + False, + ), + ( + "16", + { + "admin_users": {"user1"}, + "admin_globus_groups": {"group1"}, + }, + True, + True, + ), + ( + "17", + { + "admin_users": {"user1"}, + "admin_globus_groups": {"test-user-not-in-group"}, + }, + True, + True, + ), + ( + "18", + { + "admin_users": {"not-test-user"}, + "admin_globus_groups": {"group1"}, + }, + True, + True, + ), + ( + "19", + { + "admin_users": {"not-test-user"}, + "admin_globus_groups": {"test-user-not-in-group"}, + }, + False, + False, + ), + ], +) +async def test_globus( + globus_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.GlobusOAuthenticator = Config(class_config) + c.GlobusOAuthenticator.username_claim = "preferred_username" + authenticator = GlobusOAuthenticator(config=c) + + handled_user_model = user_model("user1") + handler = globus_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' - tokens = list(auth_model['auth_state']['tokens'].keys()) - assert tokens == ['transfer.api.globus.org'] + + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "tokens" in auth_state + assert "transfer.api.globus.org" in auth_state["tokens"] + user_info = auth_state[authenticator.user_auth_state_key] + assert auth_model["name"] == user_info[authenticator.username_claim] + if authenticator.allowed_globus_groups or authenticator.admin_globus_groups: + assert auth_state["globus_groups"] == {"group1"} + else: + assert auth_model == None async def test_globus_pre_spawn_start(mock_globus_user): @@ -225,48 +345,33 @@ async def test_globus_defaults(): async def test_restricted_domain(globus_client): - authenticator = GlobusOAuthenticator() - authenticator.identity_provider = 'alliance.gov' - handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu')) + c = Config() + c.GlobusOAuthenticator.allow_all = True + c.GlobusOAuthenticator.identity_provider = "allowed.example.com" + authenticator = GlobusOAuthenticator(config=c) + + handled_user_model = user_model("user1@example.com") + handler = globus_client.handler_for_user(handled_user_model) with raises(web.HTTPError) as exc: await authenticator.get_authenticated_user(handler, None) assert exc.value.status_code == 403 async def test_namespaced_domain(globus_client): - authenticator = GlobusOAuthenticator() - # Allow any idp - authenticator.identity_provider = '' - um = user_model('wash@legitshipping.com@serenity.com') - handler = globus_client.handler_for_user(um) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' + c = Config() + c.GlobusOAuthenticator.allow_all = True + authenticator = GlobusOAuthenticator(config=c) - -async def test_username_from_email(globus_client): - authenticator = GlobusOAuthenticator() - # Allow any idp - authenticator.identity_provider = '' - authenticator.username_from_email = True - um = user_model('wash@legitshipping.com@serenity.com', 'alan@tudyk.org') - handler = globus_client.handler_for_user(um) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'alan' - - -async def test_username_not_from_email(globus_client): - authenticator = GlobusOAuthenticator() - # Allow any idp - authenticator.identity_provider = '' - um = user_model('wash@legitshipping.com@serenity.com', 'alan@tudyk.org') - handler = globus_client.handler_for_user(um) + handled_user_model = user_model('wash@legitshipping.com@serenity.com') + handler = globus_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) assert auth_model['name'] == 'wash' -async def test_email_scope_added(globus_client): - authenticator = GlobusOAuthenticator() - authenticator.username_from_email = True +async def test_username_from_email_scope_added(globus_client): + c = Config() + c.GlobusOAuthenticator.username_from_email = True + authenticator = GlobusOAuthenticator(config=c) assert authenticator.scope == [ 'openid', 'profile', @@ -276,39 +381,53 @@ async def test_email_scope_added(globus_client): async def test_username_from_email_restricted_pass(globus_client): - authenticator = GlobusOAuthenticator() - # Allow any idp - authenticator.identity_provider = 'serenity.com' - authenticator.username_from_email = True - um = user_model('wash@serenity.com', 'alan@serenity.com') - handler = globus_client.handler_for_user(um) + c = Config() + c.GlobusOAuthenticator.allow_all = True + c.GlobusOAuthenticator.username_from_email = True + c.GlobusOAuthenticator.identity_provider = "allowed.example.com" + authenticator = GlobusOAuthenticator(config=c) + + handled_user_model = user_model( + 'dummy@example.com', email='user1@allowed.example.com' + ) + handler = globus_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'alan' + assert auth_model + assert auth_model["name"] == "user1" async def test_username_from_email_restricted_fail(globus_client): - authenticator = GlobusOAuthenticator() - # Allow any idp - authenticator.identity_provider = 'serenity.com' - authenticator.username_from_email = True - um = user_model('wash@serenity.com', 'alan@tudyk.org') - handler = globus_client.handler_for_user(um) + c = Config() + c.GlobusOAuthenticator.allow_all = True + c.GlobusOAuthenticator.username_from_email = True + c.GlobusOAuthenticator.identity_provider = "allowed.example.com" + authenticator = GlobusOAuthenticator(config=c) + + handled_user_model = user_model( + "user1@allowed.example.com", email="dummy@example.com" + ) + handler = globus_client.handler_for_user(handled_user_model) with raises(web.HTTPError) as exc: await authenticator.get_authenticated_user(handler, None) assert exc.value.status_code == 403 async def test_token_exclusion(globus_client): - authenticator = GlobusOAuthenticator() - authenticator.exclude_tokens = [ - 'transfer.api.globus.org', - 'auth.globus.org', - 'groups.api.globus.org', + c = Config() + c.GlobusOAuthenticator.allow_all = True + c.GlobusOAuthenticator.exclude_tokens = [ + "auth.globus.org", + "groups.api.globus.org", + "transfer.api.globus.org", ] - handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu')) + authenticator = GlobusOAuthenticator(config=c) + + handled_user_model = user_model("user1@example.com") + handler = globus_client.handler_for_user(handled_user_model) auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' - assert list(auth_model['auth_state']['tokens'].keys()) == [] + assert auth_model + assert auth_model['auth_state'] + assert not auth_model['auth_state']['tokens'] async def test_revoke_tokens(globus_client, mock_globus_user): @@ -395,45 +514,10 @@ async def test_logout_revokes_tokens(globus_client, monkeypatch, mock_globus_use async def test_group_scope_added(globus_client): authenticator = GlobusOAuthenticator() - authenticator.allowed_globus_groups = {'21c6bc5d-fc12-4f60-b999-76766cd596c2'} + authenticator.allowed_globus_groups = {'group-manager'} assert authenticator.scope == [ 'openid', 'profile', 'urn:globus:auth:scope:transfer.api.globus.org:all', 'urn:globus:auth:scope:groups.api.globus.org:view_my_groups_and_memberships', ] - - -async def test_user_in_allowed_group(globus_client): - authenticator = GlobusOAuthenticator() - authenticator.allowed_globus_groups = {'21c6bc5d-fc12-4f60-b999-76766cd596c2'} - handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' - - -async def test_user_not_allowed(globus_client): - authenticator = GlobusOAuthenticator() - authenticator.allowed_globus_groups = {'3f1f85c4-f084-4173-9efb-7c7e0b44291a'} - handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model == None - - -async def test_user_is_admin(globus_client): - authenticator = GlobusOAuthenticator() - authenticator.admin_globus_groups = {'21c6bc5d-fc12-4f60-b999-76766cd596c2'} - handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' - assert auth_model['admin'] == True - - -async def test_user_allowed_not_admin(globus_client): - authenticator = GlobusOAuthenticator() - authenticator.allowed_globus_groups = {'21c6bc5d-fc12-4f60-b999-76766cd596c2'} - authenticator.admin_globus_groups = {'3f1f85c4-f084-4173-9efb-7c7e0b44291a'} - handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu')) - auth_model = await authenticator.get_authenticated_user(handler, None) - assert auth_model['name'] == 'wash' - assert auth_model['admin'] == False diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py index 6aa722c7..d9e24196 100644 --- a/oauthenticator/tests/test_google.py +++ b/oauthenticator/tests/test_google.py @@ -3,7 +3,7 @@ import re from unittest import mock -from pytest import fixture, raises +from pytest import fixture, mark, raises from tornado.web import HTTPError from traitlets.config import Config @@ -11,11 +11,12 @@ from .mocks import setup_oauth_mock -def user_model(email): +def user_model(email, username="user1"): """Return a user model""" return { 'sub': hashlib.md5(email.encode()).hexdigest(), 'email': email, + 'custom': username, 'hd': email.split('@')[1], 'verified_email': True, } @@ -32,210 +33,215 @@ def google_client(client): return client -async def test_google(google_client): - authenticator = GoogleOAuthenticator() - handler = google_client.handler_for_user(user_model('fake@email.com')) +@mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + ("06", {"allowed_google_groups": {"example.com": {"group1"}}}, True, None), + ( + "07", + {"allowed_google_groups": {"example.com": {"test-user-not-in-group"}}}, + False, + None, + ), + ("08", {"admin_google_groups": {"example.com": {"group1"}}}, True, True), + ( + "09", + {"admin_google_groups": {"example.com": {"test-user-not-in-group"}}}, + False, + False, + ), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "allowed_users": {"not-test-user"}, + "admin_users": {"user1"}, + }, + True, + True, + ), + ( + "12", + { + "allowed_google_groups": {"example.com": {"group1"}}, + "admin_google_groups": {"example.com": {"group1"}}, + }, + True, + True, + ), + ( + "13", + { + "allowed_google_groups": {"example.com": {"group1"}}, + "admin_google_groups": {"example.com": {"test-user-not-in-group"}}, + }, + True, + False, + ), + ( + "14", + { + "allowed_google_groups": {"example.com": {"test-user-not-in-group"}}, + "admin_google_groups": {"example.com": {"group1"}}, + }, + True, + True, + ), + ( + "15", + { + "allowed_google_groups": {"example.com": {"test-user-not-in-group"}}, + "admin_google_groups": {"example.com": {"test-user-not-in-group"}}, + }, + False, + False, + ), + ( + "16", + { + "admin_users": {"user1"}, + "admin_google_groups": {"example.com": {"group1"}}, + }, + True, + True, + ), + ( + "17", + { + "admin_users": {"user1"}, + "admin_google_groups": {"example.com": {"test-user-not-in-group"}}, + }, + True, + True, + ), + ( + "18", + { + "admin_users": {"not-test-user"}, + "admin_google_groups": {"example.com": {"group1"}}, + }, + True, + True, + ), + ( + "19", + { + "admin_users": {"not-test-user"}, + "admin_google_groups": {"example.com": {"test-user-not-in-group"}}, + }, + False, + False, + ), + ], +) +async def test_google( + google_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.GoogleOAuthenticator = Config(class_config) + c.GoogleOAuthenticator.username_claim = "custom" + authenticator = GoogleOAuthenticator(config=c) + + handled_user_model = user_model("user1@example.com", "user1") + handler = google_client.handler_for_user(handled_user_model) with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakeadmingroup'}, + authenticator, "_fetch_user_groups", lambda *args: {"group1"} ): - user_info = await authenticator.get_authenticated_user(handler, None) - assert sorted(user_info) == ['admin', 'auth_state', 'name'] - name = user_info['name'] - assert name == 'fake@email.com' - auth_state = user_info['auth_state'] - assert 'access_token' in auth_state - assert 'google_user' in auth_state - - -async def test_google_username_claim(google_client): - cfg = Config() - cfg.GoogleOAuthenticator.username_claim = "sub" - authenticator = GoogleOAuthenticator(config=cfg) - handler = google_client.handler_for_user(user_model('fake@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakeadmingroup'}, - ): - user_info = await authenticator.get_authenticated_user(handler, None) - assert sorted(user_info) == ['admin', 'auth_state', 'name'] - name = user_info['name'] - assert name == '724f95667e2fbe903ee1b4cffcae3b25' + auth_model = await authenticator.get_authenticated_user(handler, None) + + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "access_token" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert auth_model["name"] == user_info[authenticator.username_claim] + if authenticator.allowed_google_groups or authenticator.admin_google_groups: + assert user_info["google_groups"] == {"group1"} + else: + assert auth_model == None async def test_hosted_domain(google_client): - authenticator = GoogleOAuthenticator(hosted_domain=['email.com']) - handler = google_client.handler_for_user(user_model('fake@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakeadmingroup'}, - ): - user_info = await authenticator.get_authenticated_user(handler, None) - name = user_info['name'] - assert name == 'fake@email.com' - - handler = google_client.handler_for_user(user_model('notallowed@notemail.com')) - with raises(HTTPError) as exc: - name = await authenticator.get_authenticated_user(handler, None) - assert exc.value.status_code == 403 - - -async def test_multiple_hosted_domain(google_client): - authenticator = GoogleOAuthenticator(hosted_domain=['email.com', 'mycollege.edu']) - handler = google_client.handler_for_user(user_model('fake@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakeadmingroup'}, - ): - user_info = await authenticator.get_authenticated_user(handler, None) - name = user_info['name'] - assert name == 'fake@email.com' - - handler = google_client.handler_for_user(user_model('fake2@mycollege.edu')) - user_info = await authenticator.get_authenticated_user(handler, None) - name = user_info['name'] - assert name == 'fake2@mycollege.edu' - - handler = google_client.handler_for_user(user_model('notallowed@notemail.com')) - with raises(HTTPError) as exc: - name = await authenticator.get_authenticated_user(handler, None) - assert exc.value.status_code == 403 - - -async def test_admin_google_groups(google_client): - authenticator = GoogleOAuthenticator( - hosted_domain=['email.com', 'mycollege.edu'], - admin_google_groups={'email.com': ['fakeadmingroup']}, - allowed_google_groups={'email.com': ['fakegroup']}, - ) - handler = google_client.handler_for_user(user_model('fakeadmin@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakeadmingroup'}, - ): - admin_user_info = await authenticator.get_authenticated_user(handler, None) - # Make sure the user authenticated successfully - assert admin_user_info - # Assert that the user is an admin - assert admin_user_info.get('admin', None) == True - handler = google_client.handler_for_user(user_model('fakealloweduser@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakegroup'}, - ): - allowed_user_info = await authenticator.get_authenticated_user(handler, None) - allowed_user_groups = allowed_user_info['auth_state']['google_user'][ - 'google_groups' - ] - admin_user = allowed_user_info['admin'] - assert 'fakegroup' in allowed_user_groups - assert admin_user is False - handler = google_client.handler_for_user(user_model('fakenonalloweduser@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakenonallowedgroup'}, - ): - allowed_user_groups = await authenticator.get_authenticated_user(handler, None) - assert allowed_user_groups is None - - -async def test_admin_user_but_no_admin_google_groups(google_client): - authenticator = GoogleOAuthenticator( - hosted_domain=['email.com', 'mycollege.edu'], - allowed_google_groups={'email.com': ['fakegroup']}, - admin_users=['fakeadmin@email.com'], - ) - handler = google_client.handler_for_user(user_model('fakeadmin@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakegroup'}, - ): - admin_user_info = await authenticator.get_authenticated_user(handler, data=None) - # Make sure the user authenticated successfully - assert admin_user_info - # Assert that the user is an admin - assert admin_user_info.get('admin', None) == True - - -async def test_allowed_google_groups(google_client): - authenticator = GoogleOAuthenticator( - hosted_domain=['email.com', 'mycollege.edu'], - allowed_google_groups={'email.com': ['fakegroup'], ',mycollege.edu': []}, - ) - handler = google_client.handler_for_user(user_model('fakeadmin@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakeadmingroup'}, - ): - admin_user_info = await authenticator.get_authenticated_user(handler, None) - assert admin_user_info is None - handler = google_client.handler_for_user(user_model('fakealloweduser@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakegroup'}, - ): - allowed_user_info = await authenticator.get_authenticated_user(handler, None) - allowed_user_groups = allowed_user_info['auth_state']['google_user'][ - 'google_groups' - ] - admin_field = allowed_user_info.get('admin') - assert 'fakegroup' in allowed_user_groups - assert admin_field is None - handler = google_client.handler_for_user(user_model('fakenonalloweduser@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakenonallowedgroup'}, - ): - allowed_user_groups = await authenticator.get_authenticated_user(handler, None) - assert allowed_user_groups is None - handler = google_client.handler_for_user(user_model('fake@mycollege.edu')) - with mock.patch.object( - authenticator, '_fetch_user_groups', lambda *args: {'fakegroup'} - ): - allowed_user_groups = await authenticator.get_authenticated_user(handler, None) - assert allowed_user_groups is None - - -async def test_admin_only_google_groups(google_client): - authenticator = GoogleOAuthenticator( - hosted_domain=['email.com', 'mycollege.edu'], - admin_google_groups={'email.com': ['fakeadmingroup']}, - ) - handler = google_client.handler_for_user(user_model('fakeadmin@email.com')) - with mock.patch.object( - authenticator, - '_fetch_user_groups', - lambda *args: {'anotherone', 'fakeadmingroup'}, - ): - admin_user_info = await authenticator.get_authenticated_user(handler, None) - admin_user = admin_user_info['admin'] - assert admin_user is True - - -def test_deprecated_config(caplog): - cfg = Config() - cfg.GoogleOAuthenticator.google_group_whitelist = {'email.com': ['group']} - cfg.Authenticator.whitelist = {"user1"} - - log = logging.getLogger("testlog") - authenticator = GoogleOAuthenticator(config=cfg, log=log) - assert ( - log.name, - logging.WARNING, - 'GoogleOAuthenticator.google_group_whitelist is deprecated in GoogleOAuthenticator 0.12.0, use ' - 'GoogleOAuthenticator.allowed_google_groups instead', - ) in caplog.record_tuples - - assert authenticator.allowed_google_groups == {'email.com': {'group'}} - assert authenticator.allowed_users == {"user1"} + c = Config() + c.GoogleOAuthenticator.hosted_domain = ["In-Hosted-Domain.com"] + c.GoogleOAuthenticator.allow_all = True + authenticator = GoogleOAuthenticator(config=c) + + handled_user_model = user_model("user1@iN-hosteD-domaiN.com") + handler = google_client.handler_for_user(handled_user_model) + auth_model = await authenticator.get_authenticated_user(handler, None) + assert auth_model + + handled_user_model = user_model("user1@not-in-hosted-domain.com") + handler = google_client.handler_for_user(handled_user_model) + with raises(HTTPError) as exc: + await authenticator.get_authenticated_user(handler, None) + assert exc.value.status_code == 403 + + +@mark.parametrize( + "test_variation_id,class_config,expect_config,expect_loglevel,expect_message", + [ + ( + "google_group_whitelist", + {"google_group_whitelist": {"example.com": {"dummy"}}}, + {"allowed_google_groups": {"example.com": {"dummy"}}}, + logging.WARNING, + "GoogleOAuthenticator.google_group_whitelist is deprecated in GoogleOAuthenticator 0.12.0, use GoogleOAuthenticator.allowed_google_groups instead", + ), + ], +) +async def test_deprecated_config( + caplog, + test_variation_id, + class_config, + expect_config, + expect_loglevel, + expect_message, +): + """ + Tests that a warning is emitted when using a deprecated config and that + configuring the old config ends up configuring the new config. + """ + print(f"Running test variation id {test_variation_id}") + c = Config() + c.GoogleOAuthenticator = Config(class_config) + + test_logger = logging.getLogger('testlog') + if expect_loglevel == logging.ERROR: + with raises(ValueError, match=expect_message): + GoogleOAuthenticator(config=c, log=test_logger) + else: + authenticator = GoogleOAuthenticator(config=c, log=test_logger) + for key, value in expect_config.items(): + assert getattr(authenticator, key) == value + + captured_log_tuples = caplog.record_tuples + print(captured_log_tuples) + + expected_log_tuple = (test_logger.name, expect_loglevel, expect_message) + assert expected_log_tuple in captured_log_tuples diff --git a/oauthenticator/tests/test_mediawiki.py b/oauthenticator/tests/test_mediawiki.py index 2fe751a4..ce149482 100644 --- a/oauthenticator/tests/test_mediawiki.py +++ b/oauthenticator/tests/test_mediawiki.py @@ -5,14 +5,13 @@ import jwt import requests_mock -from pytest import fixture +from pytest import fixture, mark from tornado import web +from traitlets.config import Config from ..mediawiki import AUTH_REQUEST_COOKIE_NAME, MWOAuthenticator from .mocks import mock_handler -MW_URL = 'https://meta.wikimedia.org/w/index.php' - @fixture def mediawiki(): @@ -21,7 +20,7 @@ def post_token(request, context): request_nonce = re.search(r'oauth_nonce="(.*?)"', authorization_header).group(1) content = jwt.encode( { - 'username': 'wash', + 'username': 'user1', 'aud': 'client_id', 'iss': 'https://meta.wikimedia.org', 'iat': time.time(), @@ -45,32 +44,79 @@ def post_token(request, context): yield mock -def new_authenticator(): - return MWOAuthenticator( - client_id='client_id', - client_secret='client_secret', - ) - +@mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "admin_users": {"user1"}, + "allowed_users": {"not-test-user"}, + }, + True, + True, + ), + ], +) +async def test_mediawiki( + mediawiki, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.MWOAuthenticator = Config(class_config) + c.MWOAuthenticator.client_id = "client_id" + c.MWOAuthenticator.client_secret = "client_secret" + c.MWOAuthenticator.username_claim = "username" + authenticator = MWOAuthenticator(config=c) -async def test_mediawiki(mediawiki): - authenticator = new_authenticator() handler = Mock( spec=web.RequestHandler, get_secure_cookie=Mock(return_value=json.dumps(['key', 'secret'])), request=Mock(query='oauth_token=key&oauth_verifier=me'), find_user=Mock(return_value=None), ) - user = await authenticator.get_authenticated_user(handler, None) - assert user['name'] == 'wash' - auth_state = user['auth_state'] - assert auth_state['ACCESS_TOKEN_KEY'] == 'key' - assert auth_state['ACCESS_TOKEN_SECRET'] == 'secret' - identity = auth_state['MEDIAWIKI_USER_IDENTITY'] - assert identity['username'] == user['name'] + auth_model = await authenticator.get_authenticated_user(handler, None) + + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "ACCESS_TOKEN_KEY" in auth_state + assert "ACCESS_TOKEN_SECRET" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert auth_model["name"] == user_info[authenticator.username_claim] + else: + assert auth_model == None async def test_login_redirect(mediawiki): - authenticator = new_authenticator() + authenticator = MWOAuthenticator( + client_id='client_id', + client_secret='client_secret', + ) record = [] handler = mock_handler( authenticator.login_handler, @@ -81,6 +127,6 @@ async def test_login_redirect(mediawiki): await handler.get() assert handler.get_status() == 302 assert 'Location' in handler._headers - assert handler._headers['Location'].startswith(MW_URL) + assert handler._headers['Location'].startswith(authenticator.mw_index_url) assert 'Set-Cookie' in handler._headers assert AUTH_REQUEST_COOKIE_NAME in handler._headers['Set-Cookie'] diff --git a/oauthenticator/tests/test_okpy.py b/oauthenticator/tests/test_okpy.py index 2452af08..34320284 100644 --- a/oauthenticator/tests/test_okpy.py +++ b/oauthenticator/tests/test_okpy.py @@ -1,13 +1,14 @@ -from pytest import fixture +from pytest import fixture, mark +from traitlets.config import Config from ..okpy import OkpyOAuthenticator from .mocks import no_code_test, setup_oauth_mock -def user_model(email): +def user_model(username): """Return a user model""" return { - 'email': email, + 'name': username, } @@ -23,16 +24,66 @@ def okpy_client(client): return client -async def test_okpy(okpy_client): - authenticator = OkpyOAuthenticator() - handler = okpy_client.handler_for_user(user_model('testing@example.com')) - user_info = await authenticator.get_authenticated_user(handler, None) - assert sorted(user_info) == ['admin', 'auth_state', 'name'] - name = user_info['name'] - assert name == 'testing@example.com' - auth_state = user_info['auth_state'] - assert 'access_token' in auth_state - assert 'okpy_user' in auth_state +@mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "admin_users": {"user1"}, + "allowed_users": {"not-test-user"}, + }, + True, + True, + ), + ], +) +async def test_okpy( + okpy_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.OkpyOAuthenticator = Config(class_config) + c.OkpyOAuthenticator.username_claim = "name" + authenticator = OkpyOAuthenticator(config=c) + + handled_user_model = user_model("user1") + handler = okpy_client.handler_for_user(handled_user_model) + auth_model = await authenticator.get_authenticated_user(handler, None) + + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "access_token" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert user_info == handled_user_model + assert auth_model["name"] == user_info[authenticator.username_claim] + else: + assert auth_model == None async def test_no_code(okpy_client): diff --git a/oauthenticator/tests/test_openshift.py b/oauthenticator/tests/test_openshift.py index 7ccc6587..b66a6521 100644 --- a/oauthenticator/tests/test_openshift.py +++ b/oauthenticator/tests/test_openshift.py @@ -1,18 +1,11 @@ -from pytest import fixture +import pytest +from traitlets.config import Config from ..openshift import OpenShiftOAuthenticator from .mocks import setup_oauth_mock -def user_model(username): - """Return a user model""" - return { - 'metadata': {'name': username}, - "groups": ["group1", "group2"], - } - - -@fixture +@pytest.fixture def openshift_client(client): setup_oauth_mock( client, @@ -23,83 +16,149 @@ def openshift_client(client): return client -async def test_openshift(openshift_client): - authenticator = OpenShiftOAuthenticator() - authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local" - handler = openshift_client.handler_for_user(user_model('wash')) - user_info = await authenticator.get_authenticated_user(handler, None) - assert sorted(user_info) == ['admin', 'auth_state', 'name'] - name = user_info['name'] - assert name == 'wash' - auth_state = user_info['auth_state'] - assert 'access_token' in auth_state - assert 'openshift_user' in auth_state - - -async def test_openshift_allowed_groups(openshift_client): - authenticator = OpenShiftOAuthenticator() - authenticator.allowed_groups = {'group1'} - authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local" - handler = openshift_client.handler_for_user(user_model('wash')) - user_info = await authenticator.get_authenticated_user(handler, None) - assert sorted(user_info) == ['admin', 'auth_state', 'name'] - name = user_info['name'] - assert name == 'wash' - auth_state = user_info['auth_state'] - assert 'access_token' in auth_state - assert 'openshift_user' in auth_state - groups = auth_state['openshift_user']['groups'] - assert 'group1' in groups - - -async def test_openshift_not_in_allowed_groups(openshift_client): - authenticator = OpenShiftOAuthenticator() - authenticator.allowed_groups = {'group3'} - authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local" - handler = openshift_client.handler_for_user(user_model('wash')) - user_info = await authenticator.get_authenticated_user(handler, None) - assert user_info == None - - -async def test_openshift_not_in_allowed_groups_but_is_admin(openshift_client): - authenticator = OpenShiftOAuthenticator() - authenticator.allowed_groups = {'group3'} - authenticator.admin_groups = {'group1'} - authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local" - handler = openshift_client.handler_for_user(user_model('wash')) - user_info = await authenticator.get_authenticated_user(handler, None) - assert sorted(user_info) == ['admin', 'auth_state', 'name'] - assert user_info['admin'] == True - - -async def test_openshift_in_allowed_groups_and_is_admin(openshift_client): - authenticator = OpenShiftOAuthenticator() - authenticator.allowed_groups = {'group2'} - authenticator.admin_groups = {'group1'} - authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local" - handler = openshift_client.handler_for_user(user_model('wash')) - user_info = await authenticator.get_authenticated_user(handler, None) - assert sorted(user_info) == ['admin', 'auth_state', 'name'] - assert user_info['admin'] == True - - -async def test_openshift_in_allowed_groups_and_is_not_admin(openshift_client): - authenticator = OpenShiftOAuthenticator() - authenticator.allowed_groups = {'group2'} - authenticator.admin_groups = {'group3'} - authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local" - handler = openshift_client.handler_for_user(user_model('wash')) - user_info = await authenticator.get_authenticated_user(handler, None) - assert sorted(user_info) == ['admin', 'auth_state', 'name'] - assert user_info['admin'] == False +def user_model(): + """Return a user model""" + return { + "metadata": {"name": "user1"}, + "groups": ["group1"], + } -async def test_openshift_not_in_admin_users_but_not_in_admin_groups(openshift_client): - authenticator = OpenShiftOAuthenticator() - authenticator.allowed_groups = {'group1'} - authenticator.admin_users = ['wash'] - authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local" - handler = openshift_client.handler_for_user(user_model('wash')) - user_info = await authenticator.get_authenticated_user(handler, data=None) - assert sorted(user_info) == ['admin', 'auth_state', 'name'] - assert user_info['admin'] == True +@pytest.mark.parametrize( + "test_variation_id,class_config,expect_allowed,expect_admin", + [ + # no allow config tested + ("00", {}, False, None), + # allow config, individually tested + ("01", {"allow_all": True}, True, None), + ("02", {"allowed_users": {"user1"}}, True, None), + ("03", {"allowed_users": {"not-test-user"}}, False, None), + ("04", {"admin_users": {"user1"}}, True, True), + ("05", {"admin_users": {"not-test-user"}}, False, None), + ("06", {"allowed_groups": {"group1"}}, True, None), + ("07", {"allowed_groups": {"test-user-not-in-group"}}, False, None), + ("08", {"admin_groups": {"group1"}}, True, True), + ("09", {"admin_groups": {"test-user-not-in-group"}}, False, False), + # allow config, some combinations of two tested + ( + "10", + { + "allow_all": False, + "allowed_users": {"not-test-user"}, + }, + False, + None, + ), + ( + "11", + { + "allowed_users": {"not-test-user"}, + "admin_users": {"user1"}, + }, + True, + True, + ), + ( + "12", + { + "allowed_groups": {"group1"}, + "admin_groups": {"group1"}, + }, + True, + True, + ), + ( + "13", + { + "allowed_groups": {"group1"}, + "admin_groups": {"test-user-not-in-group"}, + }, + True, + False, + ), + ( + "14", + { + "allowed_groups": {"test-user-not-in-group"}, + "admin_groups": {"group1"}, + }, + True, + True, + ), + ( + "15", + { + "allowed_groups": {"test-user-not-in-group"}, + "admin_groups": {"test-user-not-in-group"}, + }, + False, + False, + ), + ( + "16", + { + "admin_users": {"user1"}, + "admin_groups": {"group1"}, + }, + True, + True, + ), + ( + "17", + { + "admin_users": {"user1"}, + "admin_groups": {"test-user-not-in-group"}, + }, + True, + True, + ), + ( + "18", + { + "admin_users": {"not-test-user"}, + "admin_groups": {"group1"}, + }, + True, + True, + ), + ( + "19", + { + "admin_users": {"not-test-user"}, + "admin_groups": {"test-user-not-in-group"}, + }, + False, + False, + ), + ], +) +async def test_openshift( + openshift_client, + test_variation_id, + class_config, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.OpenShiftOAuthenticator = Config(class_config) + c.OpenShiftOAuthenticator.openshift_auth_api_url = ( + "https://openshift.default.svc.cluster.local" + ) + authenticator = OpenShiftOAuthenticator(config=c) + + handled_user_model = user_model() + handler = openshift_client.handler_for_user(handled_user_model) + auth_model = await authenticator.get_authenticated_user(handler, None) + + if expect_allowed: + assert auth_model + assert set(auth_model) == {"name", "admin", "auth_state"} + assert auth_model["name"] == handled_user_model["metadata"]["name"] + assert auth_model["admin"] == expect_admin + auth_state = auth_model["auth_state"] + assert "access_token" in auth_state + user_info = auth_state[authenticator.user_auth_state_key] + assert user_info == handled_user_model + else: + assert auth_model == None