Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

[AzureAD] Add config to map Azure's concept of AppRoles to conclude if a user is allowed and/or if the user is an admin #446

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

[AzureAD] Add config to map Azure's concept of AppRoles to conclude if a user is allowed and/or if the user is an admin #446

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4129ae3
Draft implementation of app roles for AzureAD
weisdd Jul 15, 2021
4fd4fb9
Reformat azuread.py
weisdd Aug 18, 2021
6dc41a6
Multiple changes:
weisdd Aug 19, 2021
4d535d6
Multiple changes:
weisdd Aug 19, 2021
22626f5
Reorder imports
weisdd Aug 19, 2021
eab9ac8
Removed the "revoke_stale_admin_privileges" feature
weisdd Aug 20, 2021
cd98b92
Removed check_user_has_role()
weisdd Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions oauthenticator/azuread.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from jupyterhub.auth import LocalAuthenticator
from tornado.httpclient import HTTPRequest
from traitlets import default
from traitlets import List
from traitlets import Unicode

from .oauth2 import OAuthenticator
Expand All @@ -19,6 +20,10 @@
PYJWT_2 = V(jwt.__version__) >= V("2.0")


def check_user_has_role(member_roles, allowed_roles):
consideRatio marked this conversation as resolved.
Show resolved Hide resolved
return any(role in member_roles for role in allowed_roles)


class AzureAdOAuthenticator(OAuthenticator):
login_service = Unicode(
os.environ.get('LOGIN_SERVICE', 'Azure AD'),
Expand All @@ -38,6 +43,18 @@ def _tenant_id_default(self):
def _username_claim_default(self):
return 'name'

admin_azure_app_roles = List(
Unicode(),
config=True,
help="App roles that should give Jupyterhub admin privileges to a user",
)

allowed_azure_app_roles = List(
Unicode(),
config=True,
help="Automatically allow users with selected app roles",
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make these to be Set instead of List. Practically, if anyone passes a List, for example via a YAML configuration that is read and passed to the configuration - then it will be automatically casted to a Set so thats fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also like to have it clarified how to list the app roles. Is it their display name rather than id? That seems a bit odd to me but that is what matches your example configuration, but perhaps it is required to be unique as well or assumed to not risk causing problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a link or something as a reference to learn more about app roles this is relevant as well? I'm not sure, but in general I'm thinking the help text seem a bit too short and could be extended with another sentence or two.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@consideRatio

I think we should make these to be Set instead of List. Practically, if anyone passes a List, for example via a YAML configuration that is read and passed to the configuration - then it will be automatically casted to a Set so thats fine.

In other implementations for IDPs, I saw both Set and List being used, so wasn't sure which of those was the best fit. I'll change that to Set as per your recommendation.

I'd also like to have it clarified how to list the app roles. Is it their display name rather than id? That seems a bit odd to me but that is what matches your example configuration, but perhaps it is required to be unique as well or assumed to not risk causing problems.

It's rather value that gets sent in the token claim. It's allowed to have the same displayName, but not the value. For the latter, Azure AD will throw an error like this:
Failed to update jupyterhub application. Error detail: It contains duplicate value. Please Provide unique value.
IDs are used by Azure AD internally and they're not exposed in the respective token claim.

Perhaps a link or something as a reference to learn more about app roles this is relevant as well? I'm not sure, but in general I'm thinking the help text seem a bit too short and could be extended with another sentence or two.

Sure, I'll think about what I can add here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to Set and improved help messages in 6dc41a6

@default("authorize_url")
def _authorize_url_default(self):
return 'https://login.microsoftonline.com/{0}/oauth2/authorize'.format(
Expand Down Expand Up @@ -94,6 +111,16 @@ async def authenticate(self, handler, data=None):
# results in a decoded JWT for the user data
auth_state['user'] = decoded

roles = decoded.get("roles", [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for readability.

Suggested change
roles = decoded.get("roles", [])
roles = auth_state['user'].get("roles", [])

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 6dc41a6


if self.admin_azure_app_roles:
if check_user_has_role(roles, self.admin_azure_app_roles):
userdict["admin"] = True

if self.allowed_azure_app_roles:
if not check_user_has_role(roles, self.allowed_azure_app_roles):
return None

return userdict


Expand Down