[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

[weisdd]

This PR is a suggested solution for https://github.com/jupyterhub/oauthenticator/issues/445 (opened by me). Since it's the first time I try to do something with Oauth2 in Python, I'd be grateful if you bear with me for all potential defects my implementation might have.

So, I suggest to rely on App roles to give admin privileges to a user and to decide whether he/she is even allowed to enter JupyterHub.

It requires the following configuration to be done on the AzureAD side:

Azure Active Directory -> App registrations -> [Name] -> Manifest:

	"appRoles": [
		{
			"allowedMemberTypes": [
				"User"
			],
			"description": "JupyterHub regular users",
			"displayName": "user",
			"id": "[RANDOM-UUID]",
			"isEnabled": true,
			"lang": null,
			"origin": "Application",
			"value": "user"
		},
		{
			"allowedMemberTypes": [
				"User"
			],
			"description": "JupyterHub admin users",
			"displayName": "admin",
			"id": "[RANDOM-UUID]",
			"isEnabled": true,
			"lang": null,
			"origin": "Application",
			"value": "admin"
		}
	],

Note: UUIDs can be generated with the help of uuidgen.

The second step would be: Enterprise applications -> [NAME] -> Users and Groups -> [assign some of the above-mentioned roles to user groups]

Then, you'll need to deploy jupyterhub (zero-to-jupyterhub) with the following values (requires to install the modified version of azuread.py):

hub:
  config:
    JupyterHub:
      admin_access: true
      authenticator_class: azuread
    Authenticator:
      admin_azure_app_roles:
        - admin
      allowed_azure_app_roles:
        - admin
        - user

After that, any user with the admin role assigned would get admin permissions and only those who have either user or admin role would be allowed to enter JupyterHub.

Additional notes:

• I'm not aware of how the whole test setup is supposed to be built, so I just replaced this code in a live container and manually started the jupyterhub process;
• I'm pretty sure I don't have exactly the same settings set for Black, so your tests are likely to fail (it'd be great if you can point me at some docs on how to make it all conformant to what you expect to see).
• It's just a draft implementation (that works in my setup), so you're more than welcomed to point at any changes in naming or main logic that need to be made.

[welcome[bot]]

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly. You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

[weisdd]

@consideRatio Could you, please, let me know who can review the PR? :) Thanks!

[consideRatio]

I updated the z2jh Helm chart config example with some notes

hub:
  config:
    JupyterHub:
      admin_access: true
      authenticator_class: azuread
    # NOTE: This must be a class that has these traitlets defined, which isn't the base Authenticator class
    AzureAdOAuthenticator:
      # NOTE: I think we should name this similar to `admin_users`, suggestion below
      #       Hmm... Not sure on the name but I think we should include "admin_users" in it
      app_roles_with_admin_users:
        - admin
      # NOTE: I think we should name this similar to `allowed_users`, suggestion below
      # NOTE: I think we should make the admin app roles be assumed to be valid users as well
      #       Hmm... Not sure on the name but I think we should include "allowed_users" in it
      app_roles_with_allowed_users:
        - user

What is an app role? Is it a list of users? Is it a property a user can have? Not sure on my naming suggestions because I don't know anything about app roles.

[consideRatio]

@weisdd thanks for the ping, sorry for the stale feedback you have received. I think this PR makes sense and isn't breaking in any way and is also an opt-in mechanism that is safe to use without breaking something else.

I consider this PR to be quite merge-ready with my comments are addressed. Oh btw, I'd also like a consideration if a test could be added to verify some logic of this or if its too complicated or similar.

Btw @weisdd and @thomafred, you have both created PRs (this, and #448) to AzureAD - perhaps you can review each others PR as well to help with the maintenance burden?

[thomafred]

This is quite neat, and I like how simple it is. It would certainly help our usecase.

A few questions:

• Will this work with B2C?
• Does the tenant require an active subscription?

[weisdd]

What is an app role? Is it a list of users? Is it a property a user can have? Not sure on my naming suggestions because I don't know anything about app roles.

@consideRatio Thanks for the review :) App roles are described here. It's very similar to client roles in keycloak. And, btw, Grafana relies on them as well (docs).

When a user authenticates via an identity provider (IDP), the IDP may add a claim (=attribute) to identity token and/or access token sent to the client. In Azure AD, identity token is used for that. The claim contains an array with role names (not IDs). Then, it's up to the application to decide whether it wants to do something with those roles. An app is free to ignore the claim if it doesn't care. Every client (=application) can have its own isolated set of roles. So, even if you're reusing the same names across multiple apps, it's absolutely fine as long as those apps have their own client (and, thus, credentials). Roles can be assigned either directly to a user or to a group the user belongs to. The beauty of this approach is the fact that you don't have to give any permissions to the client that interacts with IDP.

[weisdd]

I updated the z2jh Helm chart config example with some notes

hub:
  config:
    JupyterHub:
      admin_access: true
      authenticator_class: azuread
    # NOTE: This must be a class that has these traitlets defined, which isn't the base Authenticator class
    AzureAdOAuthenticator:
      # NOTE: I think we should name this similar to `admin_users`, suggestion below
      #       Hmm... Not sure on the name but I think we should include "admin_users" in it
      app_roles_with_admin_users:
        - admin
      # NOTE: I think we should name this similar to `allowed_users`, suggestion below
      # NOTE: I think we should make the admin app roles be assumed to be valid users as well
      #       Hmm... Not sure on the name but I think we should include "allowed_users" in it
      app_roles_with_allowed_users:
        - user

@consideRatio What do you think about this naming?

hub:
  config:
    JupyterHub:
      admin_access: true
      authenticator_class: azuread
    AzureAdOAuthenticator:
      admin_users_app_roles:
        - admin
      allowed_users_app_roles:
        - user
      revoke_stale_admin_privileges: true

I decided to add revoke_stale_admin_privileges (it's set to False by default) along the way (code). It should cover the following case: JupyterHub remembers that a user is admin meaning even if userdict["admin"] is absent during login, the admin privileges are not revoked. That means that if we change user's app role in future (e.g. admin -> user), the user will still remain admin unless someone manually revokes privileges. The flag is aimed to address that case.

        # Admin privileges will be revoked if a user is not listed in admin_users
        # and then added back if the user has one of admin_users_app_roles.
        if self.revoke_stale_admin_privileges:
            if userdict["name"].lower() not in self.admin_users:
                userdict["admin"] = False

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

A couple of notes:

• As I understand, JupyterHub always converts names to lowercase, that's why I used userdict["name"].lower() not in self.admin_users: an alternative would probably be:

        if self.revoke_stale_admin_privileges:
            admin_users = [admin.lower() for admin in self.admin_users]
            if userdict["name"].lower() not in admin_users:
                userdict["admin"] = False

• I thought that making the flag optional can give users a chance to assign privileges manually if they want to.

I consider this PR to be quite merge-ready with my comments are addressed. Oh btw, I'd also like a consideration if a test could be added to verify some logic of this or if its too complicated or similar.

Maybe we can only check the logic for assigning admin privileges, though the test is likely to be over-complicated unless the code is moved to a separate function. The latter will make the code look differently from other providers. So, not sure about that.

[weisdd]

This is quite neat, and I like how simple it is. It would certainly help our usecase.

A few questions:

• Will this work with B2C?
• Does the tenant require an active subscription?

@thomafred Unfortunately, I'm don't know Azure deep enough to answer any of those questions (I mostly used Keycloak in the past). I can only say that as long as you're using Azure AD and can create app roles and assign users/usergroups to them, it all should work just fine. :)

[manics]

As I understand, JupyterHub always converts names to lowercase

That's the default but it's not mandatory, it's handled by an overridable function: https://github.com/jupyterhub/jupyterhub/blob/59b25813705ef884e54661daf9c10bb48fa1fb54/jupyterhub/auth.py#L397-L407

[consideRatio]

Awwww haha I was so happy that this was potentially ready for merge but now there is another feature to consider and such that may not fit under the title of this PR.

@weisdd would you mind waiting with adding the revoke admin status logic into a separate PR? I think it makes a lot of sense but I think it would be hard to help readers of a changelog or similar to manage understand the changes of the next release if it bundles into this PR for example.

[weisdd]

Awwww haha I was so happy that this was potentially ready for merge but now there is another feature to consider and such that may not fit under the title of this PR.

@weisdd would you mind waiting with adding the revoke admin status logic into a separate PR? I think it makes a lot of sense but I think it would be hard to help readers of a changelog or similar to manage understand the changes of the next release if it bundles into this PR for example.

@consideRatio Ah, I had thought it would be a good idea to have it included in this PR, though you're right, better to move discussion on it to a separate PR then. I've just removed the feature :) Please, let me know if anything else should be adjusted here :)

[weisdd]

That's the default but it's not mandatory, it's handled by an overridable function: https://github.com/jupyterhub/jupyterhub/blob/59b25813705ef884e54661daf9c10bb48fa1fb54/jupyterhub/auth.py#L397-L407

@manics thanks for the info!

[weisdd]

@mafloh Could you, please, take a look at the PR? :) Thanks!

[devhell]

mafloh

Is it possible you meant @manics? :)

[weisdd]

@devhell you're right, thanks :)

@manics could you, plz, take a look at the PR? Thanks!

[weisdd]

@consideRatio I'm leaving my current company in two weeks, might not have a proper test bed later on. Any chance to either merge the PR or to request additional changes soon? :)

[Santhin]

Any updates?

[christian-sapconet]

@manics Would it possible to get this merged?