[All] breaking, add allow_all config defaulting to False (CILogon: require allowed_idps)

[consideRatio]

[All] allow_all config

When setting up a JupyterHub you may with a minimal config using an OAuthenticator based Authenticator end up allowing all authenticated users. Even if they were allowed for a brief amount of time, they can have ended up creating a user for access after login is restricted. This PR addresses this situation by defaulting to not allowing all authenticated users.

Example use

c.JupyterHub.authentication_class = "github" 
c.GitHubOAuthenticator.client_id = "..."
c.GitHubOAuthenticator.client_secret = "..."

# previously with a config like above all github users were allowed access
# now though allow_all needs to be configured explicitly for that situation
# using the allow_all config, like below
c.OAuthenticator.allow_all = True

Related

• Fixes Discussion: don't default to allowing all users? #609 and fixes [All] Stop allowing all authenticated users by default #620

[CILogon] allowed_idps required

For CILogon, it was hard to add the allow_all functionality independently from the planned change of making CILogonOAuthenticator.allowed_idps to be required. Due to that, that change is bundled in this PR.

WIth CILogonOAuthenticator.allowed_idps required, it made sense to also stop using CILogonOAuthenticator's username_claim, additional_username_claims, and shown_idps, so use of them is now also causing loud errors. The main motivation for erroring loudly instead of providing a warning is that a misconfiguration here can be a security issue.

Related

• Fixes [CILogon] Require specification of allowed_idps? #615

For the reviewer

This is a humongous PR as it was hard to avoid it but I hope you don't panic! I've thought a lot on the commits I've made, making a few relatively small commits with actual changes free from updates to comments and tests which instead are put in other larger commits. With this in mind, I recommend reviewing this commit by commit.

Here are the commits, and only three contain relevant changes, and two of those are cilogon specific changes. That makes this PR include one key commit to review (223c89b - all, breaking change: add allow_all config, default to False). When reviewing that commit, I recommend starting by looking at the oauth2.py file's changes as the other changes build on logic changed in the OAuthenticator base class.

1. 0bd5809 - (trivial) refactor, auth0: add help strings for auth0_domain and auth0_subdomain
2. 2ade700 - (trivial) refactor, globus: fix help string indentation and misc whitespace
3. 2e11e44 - cilogon, breaking change: allowed_idps must be configured
4. 223c89b - all, breaking change: add allow_all config, default to False
5. 0e2352e - (only tests/ folder touched) maint: update tests for recent change
6. 561f13c - (~only comments etc touched) maint: update comments, helpstrings, and docstrings for other commits
7. 89f7d9d - cilogon, breaking change: shown_idps, username_claim, additional_username_claims
8. 84e1aac - (trivial) maint, google: use lower case when comparing domain names
9. 032d5d2 - (trivial) maint, mediawiki: specify user_auth_state_key

Post review update

Following review, I've made the following commits so far.

1. 8d1da50 - (trivial) cilogon: switch to another status code in an error response
2. 69535f0 - (trivial) maint: update some additional docstrings
3. e7f864d - (trivial) maint, generic: refactor to at least partially rely on base class

About changes in tests

• I've added significant test coverage for allowed_all and other allow_... config by using @pytest.mark.parametrize one test per authenticator to run with a lot of different allow_ config. This parametrized test could largely be re-used across authenticators with small tweaks.
• I've removed quite a few standalone tests that were covered well via the parametrized test, and some other tests. Overall, I've been very cautious to avoid reducing the tested situations.

[GeorgianaElena]

Thank you very much @consideRatio! This looks very good and describing what each commit did, was very helpful ❤️

[consideRatio]

Thank you soo much for reviewing @GeorgianaElena!!!!

I pushed three commits, where e7f864d was not directly related to your review comments. When documenting GenericOAuthenticator, I concluded that it wad adding functionality in username_claim that the base class didn't have, and that it duplicated things done in the base class besides checking if username_claim was callable. The commit refactors away the duplicated code, now referencing the base class logic.

[GeorgianaElena]

This is amazing @consideRatio ❤️ Thank you very much for all this work and for making the review so straightforward. This is ready to be merged IMO 🚀

[consideRatio]

WIEEEEE thank you @GeorgianaElena !!!