Repository: oauth2-proxy/oauth2-proxy
Pull request #639 (2020-06-26 17:33:37)

Change how gitlab-group is parsed on options
As talked in #637, this Draft PR revolves around this comment from @JoelSpeed: https://github.com/oauth2-proxy/oauth2-proxy/issues/637#issuecomment-650116230

## Description

Currently, `gitlab-group` is parsed in a way where it is comma seperated on the command flag but space seperated 

## Motivation and Context

The current documentation does not reflect this issue.

## How Has This Been Tested?

Aside from `go test` failing for Logging for HTTP requests (which is out of scope here but logged below):

```
[2020/06/26 20:04:04] [http.go:58] HTTP: listening on 127.0.0.1:4180
[2020/06/26 20:04:04] [http.go:60] HTTP: closing 127.0.0.1:4180
--- FAIL: TestLoggingHandler_ServeHTTP (0.00s)
    logging_handler_test.go:62: Log message was
        127.0.0.1 - - [2020/06/26 20:04:04] test-server GET - "/foo/bar" HTTP/1.1 "" 200 4 0.001

        instead of matching
        127.0.0.1 - - [2020/06/26 20:04:04] test-server GET - "/foo/bar" HTTP/1.1 "" 200 4 0.000
eyJfoobar.eyJfoobar.12345asdfFAIL
exit status 1
```

I have tested the result in an already-running Gitlab oauth2 test environment and haven't gotten any errors.

## Checklist:

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.


linuxgemini: My pleasure

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f4dd76e1..b6f2d1ea 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -52,9 +52,15 @@
   - Fixes an inconsistency in the `--exclude-logging-paths` option by renaming it to `--exclude-logging-option`.
   - This flag may now be given multiple times as with other list options
   - This flag also accepts comma separated values
+- [#639](https://github.com/oauth2-proxy/oauth2-proxy/pull/639) Change how gitlab-group is parsed on options
+  - Previously, the flag gitlab-group used comma seperated values, while the config option used space seperated values.
+  - This fixes the config value to use slices internally.
+  - The config option `gitlab_group` is now `gitlab_groups`
+  - The environment variable `OAUTH2_PROXY_GITLAB_GROUP` is now `OAUTH2_PROXY_GITLAB_GROUPS`
 
 ## Changes since v5.1.1
 
+- [#639](https://github.com/oauth2-proxy/oauth2-proxy/pull/639) Change how gitlab-group is parsed on options (@linuxgemini)
 - [#615](https://github.com/oauth2-proxy/oauth2-proxy/pull/615) Kubernetes example based on Kind cluster and Nginx ingress (@EvgeniGordeev)
 - [#596](https://github.com/oauth2-proxy/oauth2-proxy/pull/596) Validate Bearer IDTokens in headers with correct provider/extra JWT Verifier (@NickMeves)
 - [#620](https://github.com/oauth2-proxy/oauth2-proxy/pull/620) Add HealthCheck middleware (@JoelSpeed)
diff --git a/docs/configuration/configuration.md b/docs/configuration/configuration.md
index dfe1e85f..96974f6f 100644
--- a/docs/configuration/configuration.md
+++ b/docs/configuration/configuration.md
@@ -57,7 +57,7 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example
 | `--github-repo` | string | restrict logins to collaborators of this repository formatted as `orgname/repo` | |
 | `--github-token` | string | the token to use when verifying repository collaborators (must have push access to the repository) | |
 | `--github-user` | string \| list | To allow users to login by username even if they do not belong to the specified org and team or collaborators | |
-| `--gitlab-group` | string | restrict logins to members of any of these groups (slug), separated by a comma | |
+| `--gitlab-group` | string \| list | restrict logins to members of any of these groups (slug), separated by a comma | |
 | `--google-admin-email` | string | the google admin to impersonate for api calls | |
 | `--google-group` | string | restrict logins to members of this google group (may be given multiple times). | |
 | `--google-service-account-json` | string | the path to the service account json credentials | |
diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go
index e3942034..ff9b1ca1 100644
--- a/pkg/apis/options/options.go
+++ b/pkg/apis/options/options.go
@@ -50,7 +50,7 @@ type Options struct {
 	GitHubRepo               string   `flag:"github-repo" cfg:"github_repo"`
 	GitHubToken              string   `flag:"github-token" cfg:"github_token"`
 	GitHubUsers              []string `flag:"github-user" cfg:"github_users"`
-	GitLabGroup              string   `flag:"gitlab-group" cfg:"gitlab_group"`
+	GitLabGroup              []string `flag:"gitlab-group" cfg:"gitlab_groups"`
 	GoogleGroups             []string `flag:"google-group" cfg:"google_group"`
 	GoogleAdminEmail         string   `flag:"google-admin-email" cfg:"google_admin_email"`
 	GoogleServiceAccountJSON string   `flag:"google-service-account-json" cfg:"google_service_account_json"`
@@ -231,7 +231,7 @@ func NewFlagSet() *pflag.FlagSet {
 	flagSet.String("github-repo", "", "restrict logins to collaborators of this repository")
 	flagSet.String("github-token", "", "the token to use when verifying repository collaborators (must have push access to the repository)")
 	flagSet.StringSlice("github-user", []string{}, "allow users with these usernames to login even if they do not belong to the specified org and team or collaborators (may be given multiple times)")
-	flagSet.String("gitlab-group", "", "restrict logins to members of this group")
+	flagSet.StringSlice("gitlab-group", []string{}, "restrict logins to members of this group (may be given multiple times)")
 	flagSet.StringSlice("google-group", []string{}, "restrict logins to members of this google group (may be given multiple times).")
 	flagSet.String("google-admin-email", "", "the google admin to impersonate for api calls")
 	flagSet.String("google-service-account-json", "", "the path to the service account json credentials")
diff --git a/pkg/validation/options.go b/pkg/validation/options.go
index c8d633c0..44a3e758 100644
--- a/pkg/validation/options.go
+++ b/pkg/validation/options.go
@@ -330,7 +330,7 @@ func parseProviderInfo(o *options.Options, msgs []string) []string {
 		}
 	case *providers.GitLabProvider:
 		p.AllowUnverifiedEmail = o.InsecureOIDCAllowUnverifiedEmail
-		p.Group = o.GitLabGroup
+		p.Groups = o.GitLabGroup
 		p.EmailDomains = o.EmailDomains
 
 		if o.GetOIDCVerifier() != nil {
diff --git a/providers/gitlab.go b/providers/gitlab.go
index 17c5df88..8d959781 100644
--- a/providers/gitlab.go
+++ b/providers/gitlab.go
@@ -18,7 +18,7 @@ import (
 type GitLabProvider struct {
 	*ProviderData
 
-	Group        string
+	Groups       []string
 	EmailDomains []string
 
 	Verifier             *oidc.IDTokenVerifier
@@ -162,7 +162,7 @@ func (p *GitLabProvider) getUserInfo(ctx context.Context, s *sessions.SessionSta
 }
 
 func (p *GitLabProvider) verifyGroupMembership(userInfo *gitlabUserInfo) error {
-	if p.Group == "" {
+	if len(p.Groups) == 0 {
 		return nil
 	}
 
@@ -173,14 +173,13 @@ func (p *GitLabProvider) verifyGroupMembership(userInfo *gitlabUserInfo) error {
 	}
 
 	// Find a valid group that they are a member of
-	validGroups := strings.Split(p.Group, " ")
-	for _, validGroup := range validGroups {
+	for _, validGroup := range p.Groups {
 		if _, ok := membershipSet[validGroup]; ok {
 			return nil
 		}
 	}
 
-	return fmt.Errorf("user is not a member of '%s'", p.Group)
+	return fmt.Errorf("user is not a member of '%s'", p.Groups)
 }
 
 func (p *GitLabProvider) verifyEmailDomain(userInfo *gitlabUserInfo) error {


Test patch:
diff --git a/providers/gitlab_test.go b/providers/gitlab_test.go
index 4a353ce8..939d634e 100644
--- a/providers/gitlab_test.go
+++ b/providers/gitlab_test.go
@@ -115,7 +115,7 @@ func TestGitLabProviderGroupMembershipValid(t *testing.T) {
 	bURL, _ := url.Parse(b.URL)
 	p := testGitLabProvider(bURL.Host)
 	p.AllowUnverifiedEmail = true
-	p.Group = "foo"
+	p.Groups = []string{"foo"}
 
 	session := &sessions.SessionState{AccessToken: "gitlab_access_token"}
 	email, err := p.GetEmailAddress(context.Background(), session)
@@ -130,7 +130,7 @@ func TestGitLabProviderGroupMembershipMissing(t *testing.T) {
 	bURL, _ := url.Parse(b.URL)
 	p := testGitLabProvider(bURL.Host)
 	p.AllowUnverifiedEmail = true
-	p.Group = "baz"
+	p.Groups = []string{"baz"}
 
 	session := &sessions.SessionState{AccessToken: "gitlab_access_token"}
 	_, err := p.GetEmailAddress(context.Background(), session)