From a172b76a8c233e39ff60fd829ff6d7df94c92764 Mon Sep 17 00:00:00 2001 From: Curtis Bangert Date: Tue, 22 Feb 2022 02:04:02 +0000 Subject: [PATCH 1/3] enhanced azure ad to include admin & allowed roles --- oauthenticator/azuread.py | 33 +++++++- oauthenticator/tests/test_azuread.py | 122 +++++++++++++++++++++++++-- 2 files changed, 146 insertions(+), 9 deletions(-) diff --git a/oauthenticator/azuread.py b/oauthenticator/azuread.py index bd7bb9ad..2801e8a3 100644 --- a/oauthenticator/azuread.py +++ b/oauthenticator/azuread.py @@ -28,6 +28,10 @@ class AzureAdOAuthenticator(OAuthenticator): tenant_id = Unicode(config=True, help="The Azure Active Directory Tenant ID") + admin_role_id = Unicode(config=True, help="The GUID of the Azure Active Directory Group containing admin users") + + allowed_user_role_id = Unicode(config=True, help="The GUID of the Azure Active Direcetory Group containing allowed users") + @default('tenant_id') def _tenant_id_default(self): return os.environ.get('AAD_TENANT_ID', '') @@ -50,6 +54,20 @@ def _token_url_default(self): self.tenant_id ) + @default('scope') + def _scope_default(self): + return ['openid'] + + role_claim = Unicode(config=True) + + @default("role_claim") + def _role_claim_default(self): + return 'roles' + + def _claim_has_role(self, token, role_id): + roles = [] if self.role_claim not in token.keys() else token[self.role_claim] + return role_id in roles + async def authenticate(self, handler, data=None): code = handler.get_argument("code") @@ -75,6 +93,8 @@ async def authenticate(self, handler, data=None): resp_json = await self.fetch(req) + self.log.debug("Azure AD Token Response: %s", resp_json) + access_token = resp_json['access_token'] id_token = resp_json['id_token'] @@ -88,14 +108,23 @@ async def authenticate(self, handler, data=None): # pyjwt 1.x decoded = jwt.decode(id_token, verify=False) + has_admin_role = self._claim_has_role(decoded, self.admin_role_id) + has_allowed_role = self._claim_has_role(decoded, self.allowed_user_role_id) or has_admin_role + allowed = has_allowed_role if self.allowed_user_role_id else True + userdict = {"name": decoded[self.username_claim]} + + if allowed: + self.log.debug("Access to Azure AD User %s is permitted (has_admin_role: %r, has_allowed_role: %r)", userdict["name"], has_admin_role, has_allowed_role) + if has_admin_role: + userdict["admin"] = has_admin_role + self.log.debug("Azure AD User %s has been granted admin privileges", userdict["name"]) userdict["auth_state"] = auth_state = {} auth_state['access_token'] = access_token # results in a decoded JWT for the user data auth_state['user'] = decoded - return userdict - + return userdict if allowed else None class LocalAzureAdOAuthenticator(LocalAuthenticator, AzureAdOAuthenticator): """A version that mixes in local system user creation""" diff --git a/oauthenticator/tests/test_azuread.py b/oauthenticator/tests/test_azuread.py index 90ce1c1b..ac537581 100644 --- a/oauthenticator/tests/test_azuread.py +++ b/oauthenticator/tests/test_azuread.py @@ -20,12 +20,11 @@ def test_tenant_id_from_env(): assert aad.tenant_id == tenant_id -def user_model(tenant_id, client_id, name): +def user_model(tenant_id, client_id, name, roles=None): """Return a user model""" # model derived from https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens#v20 now = int(time.time()) - id_token = jwt.encode( - { + token_body = { "ver": "2.0", "iss": f"https://login.microsoftonline.com/{tenant_id}/v2.0", "sub": "AAAAAAAAAAAAAAAAAAAAAIkzqFVrSaSaFHy782bbtaQ", @@ -39,9 +38,10 @@ def user_model(tenant_id, client_id, name): "tid": tenant_id, "nonce": "123523", "aio": "Df2UVXL1ix!lMCWMSOJBcFatzcGfvFGhjKv8q5g0x732dR5MB5BisvGQO7YWByjd8iQDLq!eGbIDakyp5mnOrcdqHeYSnltepQmRp6AIZ8jY", - }, - os.urandom(5), - ) + } + if roles: + token_body["roles"] = roles + id_token = jwt.encode(token_body, os.urandom(5)) if not PYJWT_2: id_token = id_token.decode("ascii") @@ -61,7 +61,6 @@ def azure_client(client): ) return client - @pytest.mark.parametrize( 'username_claim', [ @@ -98,3 +97,112 @@ async def test_azuread(username_claim, azure_client): name = user_info['name'] assert name == jwt_user[authenticator.username_claim] + +@pytest.mark.parametrize( + 'is_admin', + [ + True, + False, + ], +) +async def test_azuread_admin(is_admin, azure_client): + authenticator = AzureAdOAuthenticator( + tenant_id=str(uuid.uuid1()), + client_id=str(uuid.uuid1()), + client_secret=str(uuid.uuid1()), + admin_role_id=str(uuid.uuid1()), + ) + + roles = [] + + if is_admin: + roles.append(authenticator.admin_role_id) + + handler = azure_client.handler_for_user( + user_model( + tenant_id=authenticator.tenant_id, + client_id=authenticator.client_id, + name="somebody", + roles=(roles,None)[roles == []] + ) + ) + + user_info = await authenticator.authenticate(handler) + auth_state = user_info['auth_state'] + has_admin_role = False if 'admin' not in user_info.keys() else user_info["admin"] + + assert sorted(user_info) == ['admin','auth_state','name'] if is_admin else sorted(user_info) == ['auth_state','name'] + assert is_admin == has_admin_role + assert is_admin if has_admin_role else not is_admin + assert not is_admin if not has_admin_role else is_admin + +@pytest.mark.parametrize( + 'is_allowed', + [ + True, + False, + ], +) +@pytest.mark.parametrize( + 'is_admin', + [ + True, + False, + ], +) +@pytest.mark.parametrize( + 'allowed_role_id', + [ + "", + "somevalue", + ], +) +@pytest.mark.parametrize( + 'admin_role_id', + [ + "", + "someothervalue", + ], +) +async def test_azuread_allowed(is_allowed, is_admin, allowed_role_id, admin_role_id, azure_client): + authenticator = AzureAdOAuthenticator( + tenant_id=str(uuid.uuid1()), + client_id=str(uuid.uuid1()), + client_secret=str(uuid.uuid1()), + allowed_user_role_id=allowed_role_id, + admin_role_id=admin_role_id, + ) + + roles = [] + + if is_allowed and allowed_role_id != "": + roles.append(authenticator.allowed_user_role_id) + + if is_admin and admin_role_id != "": + roles.append(authenticator.admin_role_id) + + handler = azure_client.handler_for_user( + user_model( + tenant_id=authenticator.tenant_id, + client_id=authenticator.client_id, + name="somebody", + roles=(roles,None)[roles == []] + ) + ) + + user_info = await authenticator.authenticate(handler) + authenticated = user_info != None + auth_state = [] if not authenticated else user_info["auth_state"] + user = [] if not authenticated else auth_state["user"] + user_roles = [] if not authenticated or 'roles' not in user.keys() else user["roles"] + + has_allowed_role = allowed_role_id in user_roles + has_admin_role = admin_role_id in user_roles + allow_required = authenticator.allowed_user_role_id != "" + allowed_as_admin = (allow_required and has_admin_role) or not allow_required + allowed_as_user = (allow_required and has_allowed_role) or not allow_required + + if allowed_as_admin or allowed_as_user: + assert authenticated + elif not allowed_as_admin and not allowed_as_user: + assert not authenticated From 9255b903a804748de21251e0a9b88b1f03f0a1aa Mon Sep 17 00:00:00 2001 From: Curtis Bangert Date: Sun, 27 Feb 2022 18:18:18 +0000 Subject: [PATCH 2/3] updated to allow lists of groups --- oauthenticator/azuread.py | 46 ++++++++++++++++++---------- oauthenticator/tests/test_azuread.py | 38 ++++++++++++----------- 2 files changed, 50 insertions(+), 34 deletions(-) diff --git a/oauthenticator/azuread.py b/oauthenticator/azuread.py index 2801e8a3..595610d6 100644 --- a/oauthenticator/azuread.py +++ b/oauthenticator/azuread.py @@ -10,6 +10,7 @@ from tornado.httpclient import HTTPRequest from traitlets import default from traitlets import Unicode +from traitlets import List from .oauth2 import OAuthenticator @@ -28,9 +29,19 @@ class AzureAdOAuthenticator(OAuthenticator): tenant_id = Unicode(config=True, help="The Azure Active Directory Tenant ID") - admin_role_id = Unicode(config=True, help="The GUID of the Azure Active Directory Group containing admin users") + admin_role_ids = List( + Unicode(), + default_value=[], + config=True, + help="The GUIDs of the Azure Active Directory Groups or Application Roles containing admin users" + ) - allowed_user_role_id = Unicode(config=True, help="The GUID of the Azure Active Direcetory Group containing allowed users") + allowed_user_role_ids = List( + Unicode(), + default_value=[], + config=True, + help="The GUIDs of the Azure Active Direcetory Groups or Application Roles containing allowed users" + ) @default('tenant_id') def _tenant_id_default(self): @@ -64,9 +75,13 @@ def _scope_default(self): def _role_claim_default(self): return 'roles' - def _claim_has_role(self, token, role_id): - roles = [] if self.role_claim not in token.keys() else token[self.role_claim] - return role_id in roles + def _claim_has_role(self, token, role_ids): + if self.role_claim in token.keys(): + for role_id in role_ids: + if role_id in token[self.role_claim]: + return True + return False + async def authenticate(self, handler, data=None): code = handler.get_argument("code") @@ -108,23 +123,22 @@ async def authenticate(self, handler, data=None): # pyjwt 1.x decoded = jwt.decode(id_token, verify=False) - has_admin_role = self._claim_has_role(decoded, self.admin_role_id) - has_allowed_role = self._claim_has_role(decoded, self.allowed_user_role_id) or has_admin_role - allowed = has_allowed_role if self.allowed_user_role_id else True - userdict = {"name": decoded[self.username_claim]} - - if allowed: - self.log.debug("Access to Azure AD User %s is permitted (has_admin_role: %r, has_allowed_role: %r)", userdict["name"], has_admin_role, has_allowed_role) - if has_admin_role: - userdict["admin"] = has_admin_role - self.log.debug("Azure AD User %s has been granted admin privileges", userdict["name"]) userdict["auth_state"] = auth_state = {} auth_state['access_token'] = access_token # results in a decoded JWT for the user data auth_state['user'] = decoded - return userdict if allowed else None + all_roles = list(self.allowed_user_role_ids) + all_roles.extend(self.admin_role_ids) + if self._claim_has_role(decoded, all_roles) or self.allowed_user_role_ids == []: + self.log.debug("Access to Azure AD User %s is permitted.", userdict["name"]) + if self._claim_has_role(decoded, self.admin_role_ids): + userdict["admin"] = True + self.log.debug("Azure AD User %s has been granted admin privileges", userdict["name"]) + return userdict + + return None class LocalAzureAdOAuthenticator(LocalAuthenticator, AzureAdOAuthenticator): """A version that mixes in local system user creation""" diff --git a/oauthenticator/tests/test_azuread.py b/oauthenticator/tests/test_azuread.py index ac537581..11487407 100644 --- a/oauthenticator/tests/test_azuread.py +++ b/oauthenticator/tests/test_azuread.py @@ -110,13 +110,13 @@ async def test_azuread_admin(is_admin, azure_client): tenant_id=str(uuid.uuid1()), client_id=str(uuid.uuid1()), client_secret=str(uuid.uuid1()), - admin_role_id=str(uuid.uuid1()), + admin_role_ids=[str(uuid.uuid1())], ) roles = [] if is_admin: - roles.append(authenticator.admin_role_id) + roles.extend(authenticator.admin_role_ids) handler = azure_client.handler_for_user( user_model( @@ -151,35 +151,37 @@ async def test_azuread_admin(is_admin, azure_client): ], ) @pytest.mark.parametrize( - 'allowed_role_id', + 'allowed_role_ids', [ - "", - "somevalue", + [], + ["somevalue"], + ["somevalue","someothervalue"], ], ) @pytest.mark.parametrize( - 'admin_role_id', + 'admin_role_ids', [ - "", - "someothervalue", + [], + ["somevalue"], + ["somevalue","someothervalue"], ], ) -async def test_azuread_allowed(is_allowed, is_admin, allowed_role_id, admin_role_id, azure_client): +async def test_azuread_allowed(is_allowed, is_admin, allowed_role_ids, admin_role_ids, azure_client): authenticator = AzureAdOAuthenticator( tenant_id=str(uuid.uuid1()), client_id=str(uuid.uuid1()), client_secret=str(uuid.uuid1()), - allowed_user_role_id=allowed_role_id, - admin_role_id=admin_role_id, + allowed_user_role_ids=allowed_role_ids, + admin_role_ids=admin_role_ids, ) roles = [] - if is_allowed and allowed_role_id != "": - roles.append(authenticator.allowed_user_role_id) + if is_allowed and allowed_role_ids != []: + roles.append(authenticator.allowed_user_role_ids) - if is_admin and admin_role_id != "": - roles.append(authenticator.admin_role_id) + if is_admin and admin_role_ids != []: + roles.append(authenticator.admin_role_ids) handler = azure_client.handler_for_user( user_model( @@ -196,9 +198,9 @@ async def test_azuread_allowed(is_allowed, is_admin, allowed_role_id, admin_role user = [] if not authenticated else auth_state["user"] user_roles = [] if not authenticated or 'roles' not in user.keys() else user["roles"] - has_allowed_role = allowed_role_id in user_roles - has_admin_role = admin_role_id in user_roles - allow_required = authenticator.allowed_user_role_id != "" + has_allowed_role = [r for r in allowed_role_ids if r in user_roles] != [] + has_admin_role = [r for r in admin_role_ids if r in user_roles] != [] + allow_required = authenticator.allowed_user_role_ids != [] allowed_as_admin = (allow_required and has_admin_role) or not allow_required allowed_as_user = (allow_required and has_allowed_role) or not allow_required From 3e4d8375984d74070a1767bb8867401ea8a1ea37 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 27 Feb 2022 18:21:25 +0000 Subject: [PATCH 3/3] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- oauthenticator/azuread.py | 21 ++++++---- oauthenticator/tests/test_azuread.py | 63 ++++++++++++++++------------ 2 files changed, 49 insertions(+), 35 deletions(-) diff --git a/oauthenticator/azuread.py b/oauthenticator/azuread.py index 595610d6..d53f55b7 100644 --- a/oauthenticator/azuread.py +++ b/oauthenticator/azuread.py @@ -9,8 +9,8 @@ from jupyterhub.auth import LocalAuthenticator from tornado.httpclient import HTTPRequest from traitlets import default -from traitlets import Unicode from traitlets import List +from traitlets import Unicode from .oauth2 import OAuthenticator @@ -32,15 +32,15 @@ class AzureAdOAuthenticator(OAuthenticator): admin_role_ids = List( Unicode(), default_value=[], - config=True, - help="The GUIDs of the Azure Active Directory Groups or Application Roles containing admin users" + config=True, + help="The GUIDs of the Azure Active Directory Groups or Application Roles containing admin users", ) allowed_user_role_ids = List( Unicode(), default_value=[], - config=True, - help="The GUIDs of the Azure Active Direcetory Groups or Application Roles containing allowed users" + config=True, + help="The GUIDs of the Azure Active Direcetory Groups or Application Roles containing allowed users", ) @default('tenant_id') @@ -81,7 +81,6 @@ def _claim_has_role(self, token, role_ids): if role_id in token[self.role_claim]: return True return False - async def authenticate(self, handler, data=None): code = handler.get_argument("code") @@ -135,11 +134,15 @@ async def authenticate(self, handler, data=None): self.log.debug("Access to Azure AD User %s is permitted.", userdict["name"]) if self._claim_has_role(decoded, self.admin_role_ids): userdict["admin"] = True - self.log.debug("Azure AD User %s has been granted admin privileges", userdict["name"]) - return userdict - + self.log.debug( + "Azure AD User %s has been granted admin privileges", + userdict["name"], + ) + return userdict + return None + class LocalAzureAdOAuthenticator(LocalAuthenticator, AzureAdOAuthenticator): """A version that mixes in local system user creation""" diff --git a/oauthenticator/tests/test_azuread.py b/oauthenticator/tests/test_azuread.py index 11487407..75991a56 100644 --- a/oauthenticator/tests/test_azuread.py +++ b/oauthenticator/tests/test_azuread.py @@ -25,19 +25,19 @@ def user_model(tenant_id, client_id, name, roles=None): # model derived from https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens#v20 now = int(time.time()) token_body = { - "ver": "2.0", - "iss": f"https://login.microsoftonline.com/{tenant_id}/v2.0", - "sub": "AAAAAAAAAAAAAAAAAAAAAIkzqFVrSaSaFHy782bbtaQ", - "aud": client_id, - "exp": now + 3600, - "iat": now, - "nbf": now, - "name": name, - "preferred_username": name, - "oid": str(uuid.uuid1()), - "tid": tenant_id, - "nonce": "123523", - "aio": "Df2UVXL1ix!lMCWMSOJBcFatzcGfvFGhjKv8q5g0x732dR5MB5BisvGQO7YWByjd8iQDLq!eGbIDakyp5mnOrcdqHeYSnltepQmRp6AIZ8jY", + "ver": "2.0", + "iss": f"https://login.microsoftonline.com/{tenant_id}/v2.0", + "sub": "AAAAAAAAAAAAAAAAAAAAAIkzqFVrSaSaFHy782bbtaQ", + "aud": client_id, + "exp": now + 3600, + "iat": now, + "nbf": now, + "name": name, + "preferred_username": name, + "oid": str(uuid.uuid1()), + "tid": tenant_id, + "nonce": "123523", + "aio": "Df2UVXL1ix!lMCWMSOJBcFatzcGfvFGhjKv8q5g0x732dR5MB5BisvGQO7YWByjd8iQDLq!eGbIDakyp5mnOrcdqHeYSnltepQmRp6AIZ8jY", } if roles: token_body["roles"] = roles @@ -61,6 +61,7 @@ def azure_client(client): ) return client + @pytest.mark.parametrize( 'username_claim', [ @@ -98,6 +99,7 @@ async def test_azuread(username_claim, azure_client): name = user_info['name'] assert name == jwt_user[authenticator.username_claim] + @pytest.mark.parametrize( 'is_admin', [ @@ -112,9 +114,9 @@ async def test_azuread_admin(is_admin, azure_client): client_secret=str(uuid.uuid1()), admin_role_ids=[str(uuid.uuid1())], ) - + roles = [] - + if is_admin: roles.extend(authenticator.admin_role_ids) @@ -123,19 +125,24 @@ async def test_azuread_admin(is_admin, azure_client): tenant_id=authenticator.tenant_id, client_id=authenticator.client_id, name="somebody", - roles=(roles,None)[roles == []] + roles=(roles, None)[roles == []], ) ) user_info = await authenticator.authenticate(handler) auth_state = user_info['auth_state'] has_admin_role = False if 'admin' not in user_info.keys() else user_info["admin"] - - assert sorted(user_info) == ['admin','auth_state','name'] if is_admin else sorted(user_info) == ['auth_state','name'] + + assert ( + sorted(user_info) == ['admin', 'auth_state', 'name'] + if is_admin + else sorted(user_info) == ['auth_state', 'name'] + ) assert is_admin == has_admin_role assert is_admin if has_admin_role else not is_admin assert not is_admin if not has_admin_role else is_admin + @pytest.mark.parametrize( 'is_allowed', [ @@ -155,7 +162,7 @@ async def test_azuread_admin(is_admin, azure_client): [ [], ["somevalue"], - ["somevalue","someothervalue"], + ["somevalue", "someothervalue"], ], ) @pytest.mark.parametrize( @@ -163,10 +170,12 @@ async def test_azuread_admin(is_admin, azure_client): [ [], ["somevalue"], - ["somevalue","someothervalue"], + ["somevalue", "someothervalue"], ], ) -async def test_azuread_allowed(is_allowed, is_admin, allowed_role_ids, admin_role_ids, azure_client): +async def test_azuread_allowed( + is_allowed, is_admin, allowed_role_ids, admin_role_ids, azure_client +): authenticator = AzureAdOAuthenticator( tenant_id=str(uuid.uuid1()), client_id=str(uuid.uuid1()), @@ -176,7 +185,7 @@ async def test_azuread_allowed(is_allowed, is_admin, allowed_role_ids, admin_rol ) roles = [] - + if is_allowed and allowed_role_ids != []: roles.append(authenticator.allowed_user_role_ids) @@ -188,7 +197,7 @@ async def test_azuread_allowed(is_allowed, is_admin, allowed_role_ids, admin_rol tenant_id=authenticator.tenant_id, client_id=authenticator.client_id, name="somebody", - roles=(roles,None)[roles == []] + roles=(roles, None)[roles == []], ) ) @@ -196,12 +205,14 @@ async def test_azuread_allowed(is_allowed, is_admin, allowed_role_ids, admin_rol authenticated = user_info != None auth_state = [] if not authenticated else user_info["auth_state"] user = [] if not authenticated else auth_state["user"] - user_roles = [] if not authenticated or 'roles' not in user.keys() else user["roles"] - + user_roles = ( + [] if not authenticated or 'roles' not in user.keys() else user["roles"] + ) + has_allowed_role = [r for r in allowed_role_ids if r in user_roles] != [] has_admin_role = [r for r in admin_role_ids if r in user_roles] != [] allow_required = authenticator.allowed_user_role_ids != [] - allowed_as_admin = (allow_required and has_admin_role) or not allow_required + allowed_as_admin = (allow_required and has_admin_role) or not allow_required allowed_as_user = (allow_required and has_allowed_role) or not allow_required if allowed_as_admin or allowed_as_user: