align allowed_groups with other allowed_ config, consistent in JupyterHub 5

[minrk]

avoids relying on allow_all in jupyterhub 5.

The group is populated in authenticate and checked in check_allowed

breaking change that's technically avoidable: I moved the user_attr down a level so it is its own dict in auth_state, so it's auth_state["user_attrs"]. I could just as easily add ldap_groups within that dict, but it seems that the attributes dict has a specific meaning, so I thought it better to move it so it doesn't get any extra keys.

One test that's still failing is the search_filter test. This test passes if I set allow_all = True, but it's unclear to me if search_filter should be considered allow config or not.

closes #246

---

EDIT: auth_state is no longer the location for user attributes, they are put in auth_state["user_attributes"]

[consideRatio]

One test that's still failing is the search_filter test. This test passes if I set allow_all = True, but it's unclear to me if search_filter should be considered allow config or not.

search_filter is discussed in #265, I think it belongs to check_blocked, but this PR is an incremental improvement making allowed_user function as it should alongside allowed_groups which is great.

[consideRatio]

breaking change that's technically avoidable: I moved the user_attr down a level so it is its own dict in auth_state, so it's auth_state["user_attrs"]. I could just as easily add ldap_groups within that dict, but it seems that the attributes dict has a specific meaning, so I thought it better to move it so it doesn't get any extra keys.

👍, this seems cleaner and better long term for everyone.

[minrk]

The search_filter docstring:

LDAP3 Search Filter whose results are allowed access

suggests that this is explicit allow config, so should result in passing check_allowed. Right now, the behavior if both search_filter and allowed_groups are configured, access is permitted if both conditions are met. Typically, allow config is considered additive, but given how this works, search_filter is exclusive. So the choice is:

• keep current behavior: allowed_groups strictly narrows allowed users from search_filter
• typical understanding of allow config is additive, but this is unavailable due to how search_filter works (no user info is retrieved if search filter is not matched). So the situation of matching allowed_groups but not search_filter is unreachable.
• consider search filter sufficient if specified (closest to additive, but means allowed_groups has no effect. Could error on ignored config)

[consideRatio]

• keep current behavior: allowed_groups strictly narrows allowed users from search_filter

I frame the current behavior as: search_filter strictly narrows what users are allowed when lookup_dn is specified, no matter if they are allowed via allowed_users, admin_users, or allowed_groups.

[minrk]

OK, then shall I remove the interpretation of search_filter as allow config at all? That seems to make the most sense, based on what you've said.

[manics]

I agree. Intuitively search_filter provides a subset of users that the rest of our allowed/blocked config modifies. I don't think filtering strictly translates into our existing terminology.

[manics]

Can we also update the documentation/readme for auth_state_attributes, since the breaking change here means the attributes are moved under user_attrs. If it's helpful here's an example of how it can be used:
https://github.com/manics/zero-to-jupyterhub-k8s-examples/blob/b460cb8958bc43a68e20f225f338a59f0bc7b5a6/ldap-singleuser/jupyterhub.yml#L37

[minrk]

I think filtering maps reasonably onto "block unless" in our terminology, but we could perhaps have a better way to say that

[minrk]

ok, tests are now passing with updated descriptions. I needed to add a bit of logic to handle the default behavior for check_allowed() changing from jupyterhub 4 to 5, so the default behavior is consistent with jupyterhub itself in both versions.

[minrk]

We also have a choice on how allow_all should behave on JupyterHub 4. We can:

1. be consistent with the JupyterHub default (i.e. default switches from allow_all=True (if no auth config) in JupyterHub 4 to False in JupyterHub 5), or
2. backport JupyterHub 5 allow_all in this package (as is done in OAuthenticator) so LDAPAuthenticator behaves consistently, regardless of JupyterHub version

Any preference? I can't really decide which one is more likely to be surprising. 2 is a bigger, more breaking change, but it breaks in the safer, less permissive direction. 1. keeps current default behavior, consistent with JupyterHub itself.

[consideRatio]

We are currently having option 1, right? I think that is sufficient and allows us to move onwards.

[consideRatio]

@minrk a third approach is to remove support for JupyterHub 4 in this authenticator, which would be fine in my mind.

In order of preference, I think 3 > 1 > 2, but we could go with anything.

[consideRatio]

Thank you for working this @minrk and helping out with review @manics!!!

I propose we either drop support for JH4 as well, or merge this as it is.

[minrk]

Yes, this PR implements option 1 at the moment.

It seems quite early to drop support for JupyterHub 4, which was the latest version just a few months ago.

[consideRatio]

From mobile on a walk with a stroller, i figure we should merge now but a final review detail: is title/description still updated to reflect the PR?

[minrk]

updated title

[consideRatio]

Wieee!!