Support for group restriction for Gitlab provider #312
Conversation
|
@jehiah any plans merging this? I would love to see this merged and released. |
README.md
Outdated
| @@ -113,6 +113,10 @@ If you are using GitHub enterprise, make sure you set the following to the appro | |||
|
|
|||
| Whether you are using GitLab.com or self-hosting GitLab, follow [these steps to add an application](http://doc.gitlab.com/ce/integration/oauth_provider.html) | |||
|
|
|||
| The GitLab auth provider supports one additional parameters to restrict authentication to a specific group. Restricting by group is normally accompanied with `--email-domain=*` | |||
There was a problem hiding this comment.
nitpick: "one additional parameters" or "one additional parameters"
providers/gitlab.go
Outdated
| Group string `json:"name"` | ||
| } | ||
|
|
||
| endpoint := p.ValidateURL.Scheme + "://" + p.ValidateURL.Host + "/api/v3/groups?access_token="+accessToken |
There was a problem hiding this comment.
nitpick: spaces around +: "+accessToken
providers/gitlab.go
Outdated
| } | ||
|
|
||
| for _, group := range groups { | ||
| if( p.Group == group.Group) { |
There was a problem hiding this comment.
nitpick: no round brackets required here?
There was a problem hiding this comment.
probably best to just ask to run "gofmt"
|
I tested this feature and it works great, except if a user is a member of a lot of groups (or if they're an admin, because in that case the GitLab API returns all the groups, not just those the user is a member of). The GitLab API paginates the list of groups, so I had to add c141ff8 to make it work with more than 20 groups. |
|
So this pull request will be included in the next release? When will that be approximately? |
|
could any one merge this? |
|
We would also appreciate this feature, without this we cannot use gitlab authentication. |
|
Any news on this? |
|
Been using this in production and I can say that the code has been stable for my setup. |
|
I've also been running this in production (with c141ff8 applied) for a while now, and it's working as expected. @JacobAae, do you want to add c141ff8 to this PR, if it looks good to you? Otherwise, I can open a new PR, but I think it makes more sense to keep everything in one place. Let me know if there's anything I can do to help get this merged. |
Patch from BenoitKnecht: bitly@c141ff8
|
@BenoitKnecht I've created a patch from your commit and applied to include it in the merge request, as it looks fine. I however doubt this will ever be merged, it has been pending for years, but I know several places where it runs in production. |
|
@jehiah any chance this pull request can get merged? |
| func (p *GitLabProvider) hasGroup(accessToken string) (bool, error) { | ||
|
|
||
| var groups []struct { | ||
| Group string `json:"name"` |
There was a problem hiding this comment.
This is a serious security issue since the name of a group is only unique within it's namespace. Anyone who can create groups (usually anyone) could dupe this by creating a group with an arbitrary name and then a subgroup with the desired name.
Consider this scenario:
[
{
"id": 1,
"web_url": "https://gitlab.com/groups/admins",
"name": "admins",
"path": "admins",
"description": "",
"visibility": "private",
"lfs_enabled": true,
"avatar_url": null,
"request_access_enabled": false,
"full_name": "admins",
"full_path": "admins",
"parent_id": null
},
{
"id": 2,
"web_url": "https://gitlab.com/groups/myfunnyproject",
"name": "myfunnyproject",
"path": "myfunnyproject",
"description": "",
"visibility": "private",
"lfs_enabled": true,
"avatar_url": null,
"request_access_enabled": false,
"full_name": "myfunnyproject",
"full_path": "myfunnyproject",
"parent_id": null
},
{
"id": 3,
"web_url": "https://gitlab.com/groups/myfunnyproject/admins",
"name": "admins",
"path": "admins",
"description": "",
"visibility": "private",
"lfs_enabled": true,
"avatar_url": null,
"request_access_enabled": false,
"full_name": "myfunnyproject / myfunnyproject",
"full_path": "myfunnyproject/myfunnyproject",
"parent_id": 2
}
]There are three groups: admins, myfunnyproject and a subgroup of myfunnyproject also named admins. In the current state this would allow both members of admins and members of myfunnyproject/admins to access the site.
I suggest using full_path instead of name.
| @@ -31,6 +31,7 @@ type Options struct { | |||
| EmailDomains []string `flag:"email-domain" cfg:"email_domains"` | |||
| GitHubOrg string `flag:"github-org" cfg:"github_org"` | |||
| GitHubTeam string `flag:"github-team" cfg:"github_team"` | |||
| GitLabGroup string `flag:"gitlab-group" cfg:"gitlab_group"` | |||
There was a problem hiding this comment.
It would be nice if multiple groups could be specified, just like multiple GoogleGroups can be specified.
|
Fyi: there is an active discussion about forking this (obviously unmaintained) project here: #628 |
Should solve issue #311