Skip to content
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

Various fixes for allowed_groups and admin_groups #758

Merged
merged 12 commits into from
Sep 3, 2024

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Sep 3, 2024

Ensures allowed_groups and admin_groups works as in all authenticator classes by adding tests and resolving some implementation details. The fixes are to unreleased changes, so this is considered a maintenance PR rather than bugfix.

@consideRatio consideRatio marked this pull request as ready for review September 3, 2024 09:50
@consideRatio
Copy link
Member Author

@minrk this isn't perfect, but it catches a few issues correctly at least I think.

Do you have work in progress related to actual fixes? This PR is 100% tests and docs currently.

@@ -28,6 +28,7 @@ def user_model(username):
return {
"username": username,
"id": id,
"groups": ["group1"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to keep this test realistic? I don't think this is a real field on the user model returned from the gitlab API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now i'm happy to settle with this, but i think its worth looking up how this can be realistic for all authenticator classes responses - requires some effort though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #759 to reflect this

always applied last, not overrideable by subclasses.

Subclasses govern this behavior via `get_user_groups`
@minrk
Copy link
Member

minrk commented Sep 3, 2024

I moved the manage_groups logic outside of update_auth_model so it's unconditional, private, and not overrideable. Classes can implement get_user_groups to govern this behavior. All tests are passing. Left one comment in-line about whether the mock responses should be kept realistic, because right now we are injecting group fields that aren't going to be there. I think that's fine for the purposes of what we want to test.

@consideRatio consideRatio requested a review from minrk September 3, 2024 13:57
@consideRatio
Copy link
Member Author

@minrk I pushed a few more commits and think I'm happy with things now - go for merge?

@consideRatio consideRatio changed the title Add test coverage for allowed_groups and admin_groups Various fixes for allowed_groups and admin_groups Sep 3, 2024
@minrk minrk merged commit ad4034c into jupyterhub:main Sep 3, 2024
11 checks passed
@minrk
Copy link
Member

minrk commented Sep 3, 2024

@consideRatio looks great, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants