-
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] Added configuration: admin_role_ids, allowed_user_role_ids, role_claim #488
Conversation
def _scope_default(self): | ||
return ['openid'] | ||
|
||
role_claim = Unicode(config=True) |
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.
This should be documented with a help
string. I'm trying to review this code atm with little knowledge of AzureAD, so having a sentence or two to describe what something is about is critical.
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 are you referring to the scope part or the role_claim part?
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.
This comment was about te role_claim
part, you can pass help="documentation here"
to the Unicode()
constructor. It will end up in the documentation website then.
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.
Disregard.. it's obvious you mean the role_claim
trait.
Can you try to elaborate on this a bit? It would also be relevant to hear your thoughts about #466 as that seems to do something similar. |
It's also worth noting that JupyterHub is moving away from the idea of a single |
Understood... perhaps this approach is a little stale. At the time, the fine-grained roles were just becoming a reality. I can attempt to revise it, but I'm also curious: is there already a broader approach to oauth when it comes to fine-grained permissions onto which this implementation can be adapted? Seems like this should be approached with a bit more abstraction than just this provider alone. |
❤️ I think overall we have no clear answers, what makes sense to generalize into the Authenticator or OAuthenticator base classes, and what belongs to configuration in for example AzureAdOAuthenitcator, etc. I'm glad to hear you think along these lines, because I consider them crucial questions for this repo! I think overall, unless there is a very robust mapping from the identity provider to permissions in JupyterHub, we should lean a bit more on Authenticator.post_auth_hook that have more flexibility to do many different things. For example, with JupyterHub RBAC system coming into place in JupyterHub 2, one may want to provide some users with permissions based on details from the identity provider, but with lots of different permissions you can grant users we can't provide for example Hmm... I'm now starting to think that the main goal for an Authenticator class should be to provide information from its identity provider, but then try to rely on a general way of processing that into authorization decisions (allowed/blocked, admin/no-admin permissions, other fine grained permissions). |
I think the easiest short term solution is for authenticators to provide group memberships, with the mapping of groups to roles handled in the JupyterHub config. This is effectively what the Longer term it'd might be good for authenticators to dynamically assign RBAC permissions to a user, but as @consideRatio implies that's a much bigger design discussion. |
Sure... in #466, there seems to be some hard-coding going on with the claim in which the roles are returned. The intent behind the UPDATE This also allows the use of standard app roles in lieu of AAD groups. Some shops may prefer to use roles on the app registrations instead of AAD security groups for permissions. |
After reviewing the implementation of RBAC, it feels like authorization should be separated as a responsibility from the authenticator. Authentication and authorization should technically be separate (yet cohesive) responsibilities. The RBAC ( A general In this way, the Understandably, this could be a fairly significant refactor, but seems like a robust way to handle this problem. Another, more basic option would be to just hand off the responsibility of maintaining the various Authenticators, but this would mean re-implementing the same logic in all of them instead of centralizing it into a single interface. Not sure if you agree or not, but this is just my $0.02 :) I don't profess to be an expert, so there are probably better ways. However, if this were my personal project, I'd likely implement some sort of Authorizer-type interface to decouple things a bit more. |
With respect to this PR -- would you like me to rebase, resolve and push with lease? |
@codecae I think you've raised some very good points. Would you mind either starting an issue on the Jupyter community forum, or opening an issue on https://github.com/jupyterhub/jupyterhub/issues , with your ideas for how this should be architected? |
Enhanced azuread provider to include functionality for "admin" and "allowed user" roles.
Setup:
-OR-
c.AzureADOAuthenticator.role_claim = 'groups'
-OR-
c.AzureADOAuthenticator.role_claim
unchanged and check 'Emit groups as role claims' for ID, Access, and SAML when adding the group claim configuration.Group ID is the default setting in when configuring group claims in the app registration token configuration (most secure). You can use any other available option instead of Group ID (sAMAccountName, NetBIOSDomain\sAMAccountName, DNSDomain\sAMAccountName or On Premises Group Security Identifier) and substitute the Object IDs in
admin_role_ids
andallowed_user_role_ids
with expected values for your specific configuration.Tip: If you have an adequate plan for Azure Active Directory, you can further limit the groups that will be returned by the group claims by adding the groups to the Enterprise Application registration through the 'Manage > Users and Groups' configuration. You will also need to check the 'Groups assigned to the application' checkbox when adding the optional group claim. This is only required if the number of groups pulled for a given user is greater than 200. This is not available for the free plan Azure AD offering, as groups cannot be assigned to enterprise applications at this plan level.