-
Notifications
You must be signed in to change notification settings - Fork 365
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
Conversation
@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"], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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`
I moved the manage_groups logic outside of |
add oauth_user
@minrk I pushed a few more commits and think I'm happy with things now - go for merge? |
@consideRatio looks great, thank you! |
Ensures
allowed_groups
andadmin_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.allowed_google_groups
for example and may not work - what do we do? #756