[GitLab] possible fix to #545 : unionize allowed lists in GitLab Oauth

[floriandeboissieu]

This is a fix issue #545 : it adds the username to the set of allowed_users if it passes the allowed_gitlab_groups or the allowed_projects_ids .

I am not sure it is a good practice to edit the allowed_users set while in an async function, so I let the maintainers of oauthenticator review that solution, but it is working as expected when tested on my side.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[minrk]

I think this is the simplest path to a solution, and makes sense to me

[minrk]

Can you add allowed_users to this condition, because we don't want to start populating it if it's empty and unused?

[minrk]

Cases to check:

• if user is in blocked_users, they should stay blocked (blocked_users should be more specific than allowed_projects)
• empty allowed_users is left empty, since once it starts getting filled, it should have everyone in it.

[floriandeboissieu]

@minrk Thanks for your comments and sorry for the late answer. As you can see the version is several commits late, but I still update this patch as a temporary solution for gitlab users like me until #594 reaches a consensus and a general solution.

Can you add allowed_users to this condition, because we don't want to start populating it if it's empty and unused?

In the proposed patch, I considered that allowed_users was the only "entrypoint" of Authenticator.
The behavior I would expect is that:

• if the user is a member of allowed_gitlab_groups or allowed_gitlab_projects then it is added to allowed_users (even if allowed_users is empty at the hub startup), because I would not like to have to fill allowed_users in config.py if I wanted to invite only a group or project members.
• blocked_users takes precedence over allowed_*, in particular over allowed_users. I hope it is done that way in Authenticator (I would have to check).

[minrk]

The reason I asked for an if allowed_users check is that allowed_users is only non-empty when it's being used. An empty set means allowing all. As soon as it is non-empty, it will start excluding everything not already in the set, so adding one user to an empty set is immediately blocking everyone else. There's no reason to add the name to the empty set, so we can skip it in that case because it's already allowed.

I checked, and blocked_users does indeed take precedence, so no need to worry about that.

[consideRatio]

#594 developed to resolve this as well for all authenticators and is now merged. For GitLab it means that a user part of allowed_users, allowed_gitlab_groups, or allowed_projects_ids will be authorized access.

Thank you @floriandeboissieu for working this, and thanks for writing up a PR description that for example linked to other issues. Your work has helped me develop an understanding of the needs and behaviors for various authenticators in this project!

If you have time to contribute further, feedback on if #594 works out correctly would be great!

[floriandeboissieu]

@minrk, sorry, I did not find the time to study the cases you were suggesting...
@consideRatio, thanks for notifying me. I'll have a try as soon as possible and tell you.