-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
GitLab connector: add GitLab additional group with role #2941
Conversation
Hey @zvlb ! Thanks for the contribution! I wonder if we should make this feature more generic. For example: allow users to format group names somehow:
We can make that an incremental change probably. |
@nabokihms what do you think? |
I think is not a good idea.
|
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.
@zvlb Márk's idea is to make the feature more generic because the approach to add boolean flags for all new features scales poorly. By looking at Gitlab's doc, I can imagine at least one more boolean flag - return only groups with direct membership.
The best option is to add a way to customize how user attributes are extracted from the token, e.g., add some extended claim mappings that allow mapping Gitlab token claims to Dex token claims.
Example of what it can look like using CEL:
groups: |
claims.groups
+ claims.groups."https://gitlab.org/claims/groups/owner".map(s, s +":owner")
+ claims.groups."https://gitlab.org/claims/groups/maintainer".map(s, s +":maintainer")
+ claims.groups."https://gitlab.org/claims/groups/developer".map(s, s +":developer")
With its current implementation, the feature looks too specific to me. Other connectors, like OIDC, can also benefit from extended claim mappings.
Signed-off-by: zvlb <[email protected]>
I need some time to create unit tests |
@nabokihms @sagikazarmark Hi. Can we continue discussing this MR? |
Someone? |
I really wait for this feature in my production! |
Hello guys! We really need this PR to approve =) |
One more vote for PR |
can you merge this PR? really helpful |
Anyone? |
@zvlb thank you for the work! |
someone? |
???? |
Signed-off-by: Maksim Nabokikh <[email protected]>
Signed-off-by: Maksim Nabokikh <[email protected]>
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.
Thank you for your patience. I'm ready to merge this. LGTM!
P.S. could you please add the new option here https://dexidp.io/docs/connectors/gitlab/?
We will think about generic mutations of the token claim for connectors in the future. |
Hello. Thank you for your work! I will update the documentation tomorrow. I also have one more change in the connector that I’m using in my production environment. With this change, besides GitLab groups, it also collects projects where the user is a member. I will also submit a merge request with this functionality tomorrow. (However, this will require discussion, since to get the user’s projects, the GitLab application will need broader permissions (access to the API). This might be a controversial decision. |
Overview
Issue - #2763
In this PR I add logic to add information about group Permission for user to GitLab connector.
Before MR:
Login with Gitlab Connector
After MR (if set getGroupsPermission to
true
in connector config):If zvlb user has Owner-access to group
group1
and Maintainer-access to groupgroup2
. (And have Reporter or Guest access to group3)In GitLab, If you have access to some group, you have the same access to nested groups (oк Projects), but API request /oauth/userinfo returns information only about "main" groups. I add logic to add permission-postfix for nested groups.
What this PR does / why we need it
In my case, I want to use dex for authentification and use group-permission information to control users-access to my application.
Does this PR introduce a user-facing change?
If this PR will be applied I add information to docs