diff --git a/docs/requirements.txt b/docs/requirements.txt index 492cd526..8c7cb177 100644 --- a/docs/requirements.txt +++ b/docs/requirements.txt @@ -6,7 +6,6 @@ autodoc-traits importlib_metadata>=3.6; python_version < '3.10' myst-parser -sphinx>=2 sphinx-autobuild sphinx-book-theme sphinx-copybutton diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py index 1b03cb59..8317e776 100644 --- a/oauthenticator/cilogon.py +++ b/oauthenticator/cilogon.py @@ -123,28 +123,36 @@ def _validate_scope(self, proposal): "action": "strip_idp_domain", "domain": "utoronto.ca", }, + "allow_all": True, }, - "https://github.com/login/oauth/authorize": { + "http://google.com/accounts/o8/id": { "username_derivation": { - "username_claim": "username", + "username_claim": "email", "action": "prefix", - "prefix": "github", + "prefix": "google", }, + "allowed_domains": ["uni.edu", "something.org"], }, - "http://google.com/accounts/o8/id": { + "https://github.com/login/oauth/authorize": { "username_derivation": { - "username_claim": "username", + "username_claim": "preferred_username", "action": "prefix", - "prefix": "google", + "prefix": "github", }, - "allowed_domains": ["uni.edu", "something.org"], + # allow_all or allowed_domains not specified for ths idp, + # this means that its users must be explicitly allowed + # with a config such as allowed_users or admin_users. }, } + c.Authenticator.admin_users = ["github-user1"] + c.Authenticator.allowed_users = ["github-user2"] - Where `username_derivation` defines: + This is a description of the configuration you can pass to + `allowed_idps`. + * `username_derivation`: string (required) * `username_claim`: string (required) - The claim in the `userinfo` response from which to get the + The claim in the `userinfo` response from which to define the JupyterHub username. Examples include: `eppn`, `email`. What keys are available will depend on the scopes requested. * `action`: string @@ -158,9 +166,13 @@ def _validate_scope(self, proposal): * `prefix`: string (required if action is prefix) The prefix which will be added at the beginning of the username followed by a semi-column ":", if the action is "prefix". - * `allowed_domains`: string - It defines which domains will be allowed to login using the - specific identity provider. + * `allow_all`: bool (defaults to False) + Configuring this allows all users authenticating with this identity + provider. + * `allowed_domains`: list of strings + Configuring this together with a `username_claim` that is an email + address enables users to be allowed if their `username_claim` ends + with `@` followed by a domain in this list. .. versionchanged:: 15.0 @@ -336,14 +348,20 @@ def _get_processed_username(self, username, user_info): 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`. + Overrides the OAuthenticator.check_allowed to also allow users based on + idp specific config `allow_all` and `allowed_domains` as configured + under `allowed_idps`. """ if await super().check_allowed(username, auth_model): return True user_info = auth_model["auth_state"][self.user_auth_state_key] user_idp = user_info["idp"] + + idp_allow_all = self.allowed_idps[user_idp].get("allow_all") + if idp_allow_all: + return True + 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) @@ -351,10 +369,6 @@ async def check_allowed(self, username, auth_model): if user_domain in idp_allowed_domains: 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 diff --git a/oauthenticator/schemas/cilogon-schema.yaml b/oauthenticator/schemas/cilogon-schema.yaml index 713d2bd2..fb791599 100644 --- a/oauthenticator/schemas/cilogon-schema.yaml +++ b/oauthenticator/schemas/cilogon-schema.yaml @@ -7,6 +7,8 @@ additionalProperties: false required: - username_derivation properties: + allow_all: + type: boolean allowed_domains: type: array items: diff --git a/oauthenticator/tests/test_cilogon.py b/oauthenticator/tests/test_cilogon.py index 06f7bdc0..c638a0e0 100644 --- a/oauthenticator/tests/test_cilogon.py +++ b/oauthenticator/tests/test_cilogon.py @@ -3,7 +3,6 @@ from jsonschema.exceptions import ValidationError from pytest import fixture, mark, raises -from tornado.web import HTTPError from traitlets.config import Config from traitlets.traitlets import TraitError @@ -102,6 +101,306 @@ async def test_cilogon( assert auth_model == None +@mark.parametrize( + "test_variation_id,idp_config,class_config,test_user_name,expect_allowed,expect_admin", + [ + # test of minimal idp specific config + ( + "1 - not allowed", + { + "username_derivation": { + "username_claim": "name", + }, + }, + {}, + "user1", + False, + False, + ), + # tests of allow_all + ( + "2 - allowed by allow_all", + { + "username_derivation": { + "username_claim": "name", + }, + "allow_all": True, + }, + {}, + "user1", + True, + None, + ), + ( + "3 - not allowed by allow_all", + { + "username_derivation": { + "username_claim": "name", + }, + "allow_all": True, + }, + {}, + "user1", + True, + None, + ), + # tests of allowed_domains + ( + "4 - allowed by allowed_domains", + { + "username_derivation": { + "username_claim": "email", + }, + "allowed_domains": ["allowed-domain.org"], + }, + {}, + "user1@allowed-domain.org", + True, + None, + ), + ( + "5 - not allowed by allowed_domains", + { + "username_derivation": { + "username_claim": "email", + }, + "allowed_domains": ["allowed-domain.org"], + }, + {}, + "user1@not-allowed-domain.org", + False, + None, + ), + ( + "6 - allowed by allowed_domains (domain stripping action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "strip_idp_domain", + "domain": "allowed-domain.org", + }, + "allowed_domains": ["allowed-domain.org"], + }, + {}, + "user1@allowed-domain.org", + True, + None, + ), + ( + "7 - not allowed by allowed_domains (domain stripping action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "strip_idp_domain", + "domain": "allowed-domain.org", + }, + "allowed_domains": ["allowed-domain.org"], + }, + {}, + "user1@not-allowed-domain.org", + False, + None, + ), + ( + "8 - allowed by allowed_domains (prefix action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "prefix", + "prefix": "some-prefix", + }, + "allowed_domains": ["allowed-domain.org"], + }, + {}, + "user1@allowed-domain.org", + True, + None, + ), + ( + "9 - not allowed by allowed_domains (prefix action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "prefix", + "prefix": "some-prefix", + }, + "allowed_domains": ["allowed-domain.org"], + }, + {}, + "user1@not-allowed-domain.org", + False, + None, + ), + # test of allowed_users and admin_users together with + # username_derivation actions to verify the final usernames is what + # matters when describing allowed_users and admin_users + ( + "10 - allowed by allowed_users (domain stripping action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "strip_idp_domain", + "domain": "domain.org", + }, + }, + { + "allowed_users": ["user1"], + }, + "user1@domain.org", + True, + None, + ), + ( + "11 - allowed by admin_users (domain stripping action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "strip_idp_domain", + "domain": "domain.org", + }, + }, + { + "admin_users": ["user1"], + }, + "user1@domain.org", + True, + True, + ), + ( + "12 - allowed by allowed_users (prefix action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "prefix", + "prefix": "some-prefix", + }, + }, + { + "allowed_users": ["some-prefix:user1@domain.org"], + }, + "user1@domain.org", + True, + None, + ), + ( + "13 - allowed by admin_users (prefix action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "prefix", + "prefix": "some-prefix", + }, + }, + { + "admin_users": ["some-prefix:user1@domain.org"], + }, + "user1@domain.org", + True, + True, + ), + ( + "14 - not allowed by allowed_users (domain stripping action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "strip_idp_domain", + "domain": "domain.org", + }, + }, + { + "allowed_users": ["user1@domain.org"], + }, + "user1@domain.org", + False, + None, + ), + ( + "15 - not allowed by admin_users (domain stripping action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "strip_idp_domain", + "domain": "domain.org", + }, + }, + { + "admin_users": ["user1@domain.org"], + }, + "user1@domain.org", + False, + None, + ), + ( + "16 - not allowed by allowed_users (prefix action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "prefix", + "prefix": "some-prefix", + }, + }, + { + "allowed_users": ["user1@domain.org"], + }, + "user1@domain.org", + False, + None, + ), + ( + "17 - not allowed by admin_users (prefix action involved)", + { + "username_derivation": { + "username_claim": "email", + "action": "prefix", + "prefix": "some-prefix", + }, + }, + { + "admin_users": ["user1@domain.org"], + }, + "user1@domain.org", + False, + None, + ), + ], +) +async def test_cilogon_allowed_idps( + cilogon_client, + test_variation_id, + idp_config, + class_config, + test_user_name, + expect_allowed, + expect_admin, +): + print(f"Running test variation id {test_variation_id}") + c = Config() + c.CILogonOAuthenticator = Config(class_config) + test_idp = "https://some-idp.com/login/oauth/authorize" + c.CILogonOAuthenticator.allowed_idps = { + test_idp: idp_config, + } + authenticator = CILogonOAuthenticator(config=c) + + username_claim = idp_config["username_derivation"]["username_claim"] + handled_user_model = user_model(test_user_name, username_claim, idp=test_idp) + handler = cilogon_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 json.dumps(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 + else: + assert auth_model == None + + @mark.parametrize( "test_variation_id,class_config,expect_config,expect_loglevel,expect_message", [ @@ -302,6 +601,10 @@ async def test_config_scopes_validation(): async def test_allowed_idps_username_derivation_actions(cilogon_client): + """ + Tests all `allowed_idps[].username_derivation.action` config choices: + `strip_idp_domain`, `prefix`, and no action specified. + """ c = Config() c.CILogonOAuthenticator.allow_all = True c.CILogonOAuthenticator.allowed_idps = { @@ -316,7 +619,7 @@ async def test_allowed_idps_username_derivation_actions(cilogon_client): 'username_derivation': { 'username_claim': 'nickname', 'action': 'prefix', - 'prefix': 'idp', + 'prefix': 'some-prefix', }, }, 'https://no-action.example.com/login/oauth/authorize': { @@ -359,7 +662,7 @@ async def test_allowed_idps_username_derivation_actions(cilogon_client): ) auth_model = await authenticator.get_authenticated_user(handler, None) print(json.dumps(auth_model, sort_keys=True, indent=4)) - assert auth_model['name'] == 'idp:jtkirk' + assert auth_model['name'] == 'some-prefix:jtkirk' # Test no action handler = cilogon_client.handler_for_user( @@ -372,87 +675,3 @@ async def test_allowed_idps_username_derivation_actions(cilogon_client): auth_model = await authenticator.get_authenticated_user(handler, None) print(json.dumps(auth_model, sort_keys=True, indent=4)) assert auth_model['name'] == 'jtkirk' - - -async def test_not_allowed_domains_and_stripping(cilogon_client): - c = Config() - c.CILogonOAuthenticator.allowed_idps = { - 'https://some-idp.com/login/oauth/authorize': { - 'username_derivation': { - 'username_claim': 'email', - 'action': 'strip_idp_domain', - 'domain': 'uni.edu', - }, - 'allowed_domains': ['pink.org'], - }, - } - - authenticator = CILogonOAuthenticator(config=c) - - # Test stripping domain not allowed - handler = cilogon_client.handler_for_user( - user_model( - 'jtkirk@uni.edu', 'email', idp='https://some-idp.com/login/oauth/authorize' - ) - ) - - # The domain to be stripped isn't allowed, so it should fail - with raises(HTTPError): - await authenticator.get_authenticated_user(handler, None) - - -async def test_allowed_domains_and_stripping(cilogon_client): - c = Config() - c.CILogonOAuthenticator.allowed_idps = { - 'https://some-idp.com/login/oauth/authorize': { - 'username_derivation': { - 'username_claim': 'email', - 'action': 'strip_idp_domain', - 'domain': 'pink.org', - }, - 'allowed_domains': ['pink.org'], - }, - } - - authenticator = CILogonOAuthenticator(config=c) - - # Test stripping allowed domain - handler = cilogon_client.handler_for_user( - 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' - - -async def test_allowed_domains_no_stripping(cilogon_client): - c = Config() - c.CILogonOAuthenticator.allowed_idps = { - 'https://some-idp.com/login/oauth/authorize': { - 'username_derivation': { - 'username_claim': 'email', - }, - 'allowed_domains': ['pink.org'], - }, - } - - authenticator = CILogonOAuthenticator(config=c) - - # Test login with user not part of allowed_domains - handler = cilogon_client.handler_for_user( - 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 login with part of allowed_domains - handler = cilogon_client.handler_for_user( - 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'