-
Notifications
You must be signed in to change notification settings - Fork 367
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.
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
[AzureAD] Add an implementation for Authenticator.authenticator_managed_groups=true #448
Conversation
Signed-off-by: Thomas Li Fredriksen <thomas.fredriksen@oceandata.earth>
Thanks for submitting your first pull request! You are awesome! 🤗 |
def load_user_groups(self, auth_state): | ||
auth_user_state = auth_state["auth_state"]["user"] | ||
return auth_user_state[self.user_groups_claim] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
…configs Signed-off-by: Thomas Li Fredriksen <thomas.fredriksen@oceandata.earth>
Signed-off-by: Thomas Li Fredriksen <thomas.fredriksen@oceandata.earth>
@default('user_groups_claim') | ||
def _user_groups_claim_default(self): | ||
return 'groups' |
There was a problem hiding this comment.
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.
Abandoned in favor of this PR: #573 |
See this related PR in jupyterhub/jupyterhub-repo.
Added support for external user group management to Azure AD authenticator.
Usage example: