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

GitLab connector: add GitLab additional group with role #2941

Merged
merged 14 commits into from
Oct 24, 2024

Conversation

zvlb
Copy link
Contributor

@zvlb zvlb commented May 11, 2023

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

time="2023-05-11T12:01:02Z" level=info msg="login successful: connector "gitlab", username="Vladimir Zemtsov", preferred_username="zvlb", email="***", groups=["group1", "group1/group1.1", "group1/group1.2", "group2", "group3"]
After MR (if set getGroupsPermission to true in connector config):
time="2023-05-11T12:05:22Z" level=info msg="login successful: connector "gitlab", username="Vladimir Zemtsov", preferred_username="zvlb", email="***", groups=["group1", "group1/group1.1", "group1/group1.2", "group2", "group3", "group1:owner", "group1/group1.1:owner", "group1/group1.2:owner", "group2:maintainer"]

If zvlb user has Owner-access to group group1 and Maintainer-access to group group2. (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

GitLab connector: add GitLab additional group with a role

@sagikazarmark
Copy link
Member

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:

  • %name%
  • %name%:%role%
  • %name%;%role%
  • %name_full_path%

We can make that an incremental change probably.

@sagikazarmark
Copy link
Member

@nabokihms what do you think?

@zvlb
Copy link
Contributor Author

zvlb commented May 12, 2023

I wonder if we should make this feature more generic. For example: allow users to format group names somehow:

  • %name%
  • %name%:%role%
  • %name%;%role%
  • %name_full_path%

I think is not a good idea. %name% - it's GitLab group name? But in GitLab, we can have groups with the same name, but different paths. For example:

  • /group1/test-group
  • /group2/test-group
    In this case, we have 2 groups with the name test-group, but it's different groups with different permission

Copy link
Member

@nabokihms nabokihms left a 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.

connector/gitlab/gitlab.go Show resolved Hide resolved
connector/gitlab/gitlab.go Show resolved Hide resolved
connector/gitlab/gitlab.go Show resolved Hide resolved
@zvlb
Copy link
Contributor Author

zvlb commented Jun 1, 2023

I need some time to create unit tests

@zvlb zvlb requested a review from nabokihms June 12, 2023 12:49
@zvlb
Copy link
Contributor Author

zvlb commented Jun 20, 2023

@nabokihms @sagikazarmark Hi. Can we continue discussing this MR?

@zvlb
Copy link
Contributor Author

zvlb commented Jul 13, 2023

Someone?

@dolgovas
Copy link

I really wait for this feature in my production!

@progmatv
Copy link

Hello guys! We really need this PR to approve =)

@anatolychernov
Copy link

One more vote for PR

@svbrsn
Copy link

svbrsn commented Aug 3, 2023

can you merge this PR? really helpful

@zvlb
Copy link
Contributor Author

zvlb commented Aug 8, 2023

Anyone?
@nabokihms
@sagikazarmark

@eduherraiz
Copy link

@zvlb thank you for the work!
This looks very promising for me to manage argo roles based on gitlab's groups permissions.
Is there any way to merge this and do some other future efforts to be more generic for use in other connectors?
cc @nabokihms @sagikazarmark

@zvlb
Copy link
Contributor Author

zvlb commented Apr 10, 2024

someone?

@zvlb
Copy link
Contributor Author

zvlb commented Jul 18, 2024

????

Signed-off-by: Maksim Nabokikh <[email protected]>
Copy link
Member

@nabokihms nabokihms left a 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/?

@nabokihms nabokihms merged commit 3e00d33 into dexidp:master Oct 24, 2024
9 checks passed
@nabokihms
Copy link
Member

We will think about generic mutations of the token claim for connectors in the future.

@zvlb
Copy link
Contributor Author

zvlb commented Oct 24, 2024

@nabokihms

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.
(Currently, for Dex to work, only access to GitLab userinfo is required.)

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

Successfully merging this pull request may close these issues.

8 participants