New group config in OAuthenticator overlaps with `allowed_google_groups` for example and may not work - what do we do?

[consideRatio]

735 moved group related config in Generic to OAuthenticator, and did a breaking change related to that group config previosly found only in GenericOAuthenticator.

Documentation about breaking change from #735

> ## Backwards compatibility > * The existing `claim_groups_key` behavior is preserved, by being passed on to `auth_state_groups_key` in the base. It has been marked as deprecated. This is not a backwards compatibility break. > * All group related functionality now **requires** `manage_groups` to be `True`, which was not the case earlier. Before this, if `manage_groups` is false but any of the group related authorization functionality (`allowed_groups` and `admin_groups`) is used, they control group related behavior **but don't show up as JupyterHub groups**. This causes confusion, as the 'groups' field in the admin panel will be empty, and possible other group related behavior (such as future profile list filtering, for example) would _not_ respect these groups. We basically would end up with _two_ group concepts - First class JupyterHub groups (which will show up in admin panel, API, can be _edited_ by admins, etc) as well as second class 'Authenticator' groups (which are only used for authorization and 'disappear' after that). I think this is unnecessary complication, and this is a good time to remove this distinction. Now, any kind of group related authorization functionality _requires_ `manage_groups` to be `True`, and we are back to having only one notion of 'group'. We also remove the confusing part where you may have `allowed_groups` set to something, manually modify the groups the user is a part of in JupyterHub admin, and it silently has no effect. This is a _breaking change_ for people who used groups functionality but set `manage_groups` to be `False`. However, I think that usage is fairly minor, because of the confusing behavior it causes. I have added the 'breaking' label here regardless. > > ## Breaking change > * All group related functionality (`allowed_groups`, `admin_groups`, `claims_group_key` and `auth_state_groups_key`) now also requires `manage_groups` to be set to `True`

We have two situations to address in my mind:

• Some classes have two sets of group config without guidance on whats supported and recommended to use.

  For example, these configs are available side by side with allowed_groups now.

  GitHubOAuthenticator.allowed_organizations GitLabOAuthenticator.allowed_gitlab_groups GlobusOAuthenticator.allowed_globus_groups GoogleOAuthenticator.allowed_google_groups

• New set of group config may not work consistently across authenticator classes

  Searching for def update_auth_model in the code base, it seems we typically don't call the base class implementation, making logic required for the new set of group config.

  Tests were updated in Generic and OpenShift, but not in other classes such as GitHub, GitLab, Globus, Google.

[minrk]

While they are related, I'm not sure they overlap. But we should definitely communicate about them better! The difference is that allowed_groups refers to JupyterHub groups (the groups key returned in the auth model), while the allowed_globus_groups, orgs, etc. refer to membership in something in the provider, which may or may not (usually not) relate to JupyterHub groups.

There are two categories of config here:

• allowed_organizations and equivalents, which affect the return value of check_allowed, but do not affect the groups returned by authenticate, etc. allowed_groups config does not enter in here.
• Authenticator.manage_groups is a mode where OAuthenticator classes may set JupyterHub group membership. This could be derived from the above memberships, or not. It's not specified and not implemented by default in any implementation. Only when manage_groups is True, does Authenticator.allowed_groups have any effect (setting it without manage_groups will prevent JupyterHub startup with an informative error.

I'll take a stab at a docs update. I don't think there's a behavior problem.

[minrk]

Hopefully #757 clarifies some things, but please feel free to point out anything that you think still needs clarifying or should have an example.

[consideRatio]

Thank you @minrk for clarifying things!

Before closing this, I'd like to add test that this still works as intended with various additional authenticator classes than just Generic and OpenShift.

[minrk]

I'm not sure I follow what you would like a test for, can you give an example config? We do already have tests for the other Authenticator classes allowed_ config, that's why they didn't need updating, because they still work exactly as before. manage_groups and allowed_groups don't interact with the prior config not related to JupyterHub groups in any way, so tests for the two unrelated features are entirely separate.

[consideRatio]

• Searching for def update_auth_model in the code base, it seems we typically don't call the base class implementation, making logic required for the new set of group config. Tests were updated in Generic and OpenShift, but not in other classes such as GitHub, GitLab, Globus, Google.

Even if things are unrelated, I don't trust yet that the implementation works for all authenticators because I saw that update_auth_model when overridden in some classes didn't call the super() implementation, and logic was put there related to allowed_groups and admin_groups.

I'll spend time looking into this tomorrow I hope, but I'd like to have basic tests related to allowed_groups and admin_groups.

[minrk]

Got it! I'll make sure there are tests that cover both allowed_groups and allowed_organizations-type config to look for interactions.

[minrk]

...and you're absolutely right! The lack of super calls in update_auth_model() is going to result in some config getting ignored. Good catch! I somehow missed that sentence in your original description, which clarifies a lot for me.

[minrk]

I'm stuck a bit API-design-wise. In 16.x, update_auth_model is generally empty in the base class, and totally controlled by subclasses (no super). Right now, the base update_auth_model applies manage_groups configuration, which should generally be done at the end of any subclass's update_auth_model, because the manage_groups behavior will likely need to take into account the results of update_auth_model. Calling super at the end is unusual, and can lead to problems when you have more than one level of inheritance:

class A:
    def method(self):
        do_something_with_subclass_info()

class B(A):
    def method(self):
        populate_something()
        return super().method()

class C(B):
    def method(self):
        # how do I inject something between populate_something and A.method()?

That makes me think that we should define the base update_auth_model as empty (as seen in most overrideable tornado methods like prepare, etc.), and apply manage_groups in a (private?) method afterward.

[consideRatio]

I'm not sure either atm, but I can come back and think -- for now lunch break, I'm not sure when I find time to work further but hope to have it later today.

[consideRatio]

I did some checks against all authenticators, and my conclusion is that this is whats to be communicated in the changelog for v17:

• AzureAd:
  • deprecation: user_groups_claim is deprecated in favor of auth_state_groups_key
• Generic:
  • deprecation: claim_groups_key is deprecated in favor of auth_state_groups_key
  • breaking: allowed_groups and admin_groups now require manage_groups
• OpenShift:
  • breaking: allowed_groups and admin_groups now require manage_groups