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 an implementation for Authenticator.authenticator_managed_groups=true #448

Closed
wants to merge 3 commits into from
Closed

[AzureAD] Add an implementation for Authenticator.authenticator_managed_groups=true #448

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54ccec1
Azure AD - Added support for external user group management
thomafred Jul 23, 2021
1046287
Azure AD - Added description to username_claim and user_groups_claim …
thomafred Aug 19, 2021
e1b0d6b
Azure AD - Fixed failing pre-commit test
thomafred Aug 19, 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
14 changes: 13 additions & 1 deletion oauthenticator/azuread.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,20 @@ class AzureAdOAuthenticator(OAuthenticator):
def _tenant_id_default(self):
return os.environ.get('AAD_TENANT_ID', '')

username_claim = Unicode(config=True)
username_claim = Unicode(config=True, help="Name of claim containing username")

@default('username_claim')
def _username_claim_default(self):
return 'name'

user_groups_claim = Unicode(
config=True, help="Name of claim containing user group memberships"
)

@default('user_groups_claim')
def _user_groups_claim_default(self):
return 'groups'
Comment on lines +45 to +47
Copy link
Member

Choose a reason for hiding this comment

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

If you have a default that is fixed like this, you can have it as the first argument passed to the Unicode constructor above instead.


@default("authorize_url")
def _authorize_url_default(self):
return 'https://login.microsoftonline.com/{0}/oauth2/authorize'.format(
Expand Down Expand Up @@ -96,6 +104,10 @@ async def authenticate(self, handler, data=None):

return userdict

def load_user_groups(self, auth_state):
auth_user_state = auth_state["auth_state"]["user"]
return auth_user_state[self.user_groups_claim]
Comment on lines +107 to +109
Copy link
Member

Choose a reason for hiding this comment

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

Can we assume auth_user_state always include a claim named groups? Does it imply that you have requested the scope groups as well or not?

Copy link
Member

Choose a reason for hiding this comment

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

Is the load_user_groups function an Authenticator base class function that is overridden?

Copy link
Member

@consideRatio consideRatio Aug 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm, what is calling this function? I find no reference to it in this repo or the Authenticator base class definition.

Also I found no reference to the authenticator_managed_groups configuration that is configuredin the configuration example you provide within the Authenticator base class.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this PR relies on jupyterhub/jupyterhub#3548.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@consideRatio - Unsure whether auth_user_state will return with a groups-claim.

The functionality in this PR requires auth_users_state to return some claim with a list of groups if c.Authenticator.authenticator_managed_groups = True. The claim name itself is configurable.

Copy link

Choose a reason for hiding this comment

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

The groups-claim is non standard for OIDC, but AAD makes it available as an optional claim
https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-optional-claims.

In our case we extend our AAD's policy to make API calls to fetch and return group membership, as shown in this example: https://github.com/azure-ad-b2c/samples/tree/master/policies/rest-api-idp



class LocalAzureAdOAuthenticator(LocalAuthenticator, AzureAdOAuthenticator):
"""A version that mixes in local system user creation"""
Expand Down