From e75a258299ec3db633450dd48a6df54b38988916 Mon Sep 17 00:00:00 2001 From: Sourav Agrawal <146818014+sourava01@users.noreply.github.com> Date: Thu, 24 Jul 2025 11:25:54 +0530 Subject: [PATCH] feat: make google-groups argument optional (#3138) add test cases update documentation refactor code and some cleanup update changelog Signed-off-by: Jan Larwig --- CHANGELOG.md | 1 + docs/docs/configuration/providers/google.md | 2 +- pkg/validation/options_test.go | 26 +++++- pkg/validation/providers.go | 6 +- providers/google.go | 89 +++++++++++++++++---- providers/google_test.go | 36 +++++++++ 6 files changed, 135 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78718cc3..0c117129 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ - [#2524](https://github.com/oauth2-proxy/oauth2-proxy/pull/2524) fix: regex substitution for $ signs in upstream path handling before running envsubst (@dashkan / @tuunit) - [#3104](https://github.com/oauth2-proxy/oauth2-proxy/pull/3104) feat(cookie): add feature support for cookie-secret-file (@sandy2008) - [#3055](https://github.com/oauth2-proxy/oauth2-proxy/pull/3055) feat: support non-default authorization request response mode also for OIDC providers (@stieler-it) +- [#3138](https://github.com/oauth2-proxy/oauth2-proxy/pull/3138) feat: make google_groups argument optional when using google provider (@sourava01) # V7.10.0 diff --git a/docs/docs/configuration/providers/google.md b/docs/docs/configuration/providers/google.md index 26af87ab..ac2a7dfa 100644 --- a/docs/docs/configuration/providers/google.md +++ b/docs/docs/configuration/providers/google.md @@ -8,7 +8,7 @@ title: Google (default) | Flag | Toml Field | Type | Description | Default | | ---------------------------------------------- | -------------------------------------------- | ------ | ------------------------------------------------------------------------------------------------ | -------------------------------------------------- | | `--google-admin-email` | `google_admin_email` | string | the google admin to impersonate for api calls | | -| `--google-group` | `google_groups` | string | restrict logins to members of this google group (may be given multiple times). | | +| `--google-group` | `google_groups` | string | restrict logins to members of this google group (may be given multiple times). If not specified and service account or default credentials are configured, all user groups will be allowed. | | | `--google-service-account-json` | `google_service_account_json` | string | the path to the service account json credentials | | | `--google-use-application-default-credentials` | `google_use_application_default_credentials` | bool | use application default credentials instead of service account json (i.e. GKE Workload Identity) | | | `--google-target-principal` | `google_target_principal` | bool | the target principal to impersonate when using ADC | defaults to the service account configured for ADC | diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index 5c242e02..5c283545 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -55,18 +55,38 @@ func TestNewOptions(t *testing.T) { assert.Equal(t, expected, err.Error()) } -func TestGoogleGroupOptions(t *testing.T) { +func TestGoogleGroupOptionsWithoutServiceAccountJSON(t *testing.T) { o := testOptions() - o.Providers[0].GoogleConfig.Groups = []string{"googlegroup"} + o.Providers[0].GoogleConfig.AdminEmail = "admin@example.com" err := Validate(o) assert.NotEqual(t, nil, err) expected := errorMsg([]string{ - "missing setting: google-admin-email", "missing setting: google-service-account-json or google-use-application-default-credentials"}) assert.Equal(t, expected, err.Error()) } +func TestGoogleGroupOptionsWithoutAdminEmail(t *testing.T) { + o := testOptions() + o.Providers[0].GoogleConfig.UseApplicationDefaultCredentials = true + err := Validate(o) + assert.NotEqual(t, nil, err) + + expected := errorMsg([]string{ + "missing setting: google-admin-email"}) + assert.Equal(t, expected, err.Error()) +} + +func TestGoogleGroupOptionsWithoutGroups(t *testing.T) { + o := testOptions() + // Set admin email and application default credentials but no groups - should still require them + o.Providers[0].GoogleConfig.AdminEmail = "admin@example.com" + o.Providers[0].GoogleConfig.UseApplicationDefaultCredentials = true + err := Validate(o) + // Should pass validation since google-group is now optional + assert.Equal(t, nil, err) +} + func TestGoogleGroupInvalidFile(t *testing.T) { o := testOptions() o.Providers[0].GoogleConfig.Groups = []string{"test_group"} diff --git a/pkg/validation/providers.go b/pkg/validation/providers.go index b1106b35..4527b841 100644 --- a/pkg/validation/providers.go +++ b/pkg/validation/providers.go @@ -94,18 +94,14 @@ func validateClientSecret(provider options.Provider) []string { func validateGoogleConfig(provider options.Provider) []string { msgs := []string{} - hasGoogleGroups := len(provider.GoogleConfig.Groups) >= 1 hasAdminEmail := provider.GoogleConfig.AdminEmail != "" hasSAJSON := provider.GoogleConfig.ServiceAccountJSON != "" useADC := provider.GoogleConfig.UseApplicationDefaultCredentials - if !hasGoogleGroups && !hasAdminEmail && !hasSAJSON && !useADC { + if !hasAdminEmail && !hasSAJSON && !useADC { return msgs } - if !hasGoogleGroups { - msgs = append(msgs, "missing setting: google-group") - } if !hasAdminEmail { msgs = append(msgs, "missing setting: google-admin-email") } diff --git a/providers/google.go b/providers/google.go index 0e1e2156..097e3567 100644 --- a/providers/google.go +++ b/providers/google.go @@ -103,17 +103,24 @@ func NewGoogleProvider(p *ProviderData, opts options.GoogleOptions) (*GoogleProv } if opts.ServiceAccountJSON != "" || opts.UseApplicationDefaultCredentials { - // Backwards compatibility with `--google-group` option - if len(opts.Groups) > 0 { - provider.setAllowedGroups(opts.Groups) - } - - provider.setGroupRestriction(opts) + provider.configureGroups(opts) } return provider, nil } +func (p *GoogleProvider) configureGroups(opts options.GoogleOptions) { + adminService := getAdminService(opts) + // Backwards compatibility with `--google-group` option + if len(opts.Groups) > 0 { + p.setAllowedGroups(opts.Groups) + p.groupValidator = p.setGroupRestriction(opts.Groups, adminService) + return + } + + p.groupValidator = p.populateAllGroups(adminService) +} + func claimsFromIDToken(idToken string) (*claims, error) { // id_token is a base64 encode ID token payload @@ -209,18 +216,13 @@ func (p *GoogleProvider) EnrichSession(_ context.Context, s *sessions.SessionSta } // SetGroupRestriction configures the GoogleProvider to restrict access to the -// specified group(s). AdminEmail has to be an administrative email on the domain that is -// checked. CredentialsFile is the path to a json file containing a Google service -// account credentials. -// -// TODO (@NickMeves) - Unit Test this OR refactor away from groupValidator func -func (p *GoogleProvider) setGroupRestriction(opts options.GoogleOptions) { - adminService := getAdminService(opts) - p.groupValidator = func(s *sessions.SessionState) bool { +// specified group(s). +func (p *GoogleProvider) setGroupRestriction(groups []string, adminService *admin.Service) func(*sessions.SessionState) bool { + return func(s *sessions.SessionState) bool { // Reset our saved Groups in case membership changed // This is used by `Authorize` on every request - s.Groups = make([]string, 0, len(opts.Groups)) - for _, group := range opts.Groups { + s.Groups = make([]string, 0, len(groups)) + for _, group := range groups { if userInGroup(adminService, group, s.Email) { s.Groups = append(s.Groups, group) } @@ -229,6 +231,25 @@ func (p *GoogleProvider) setGroupRestriction(opts options.GoogleOptions) { } } +// populateAllGroups configures the GoogleProvider to allow access with all +// groups and populate session with all groups of the user when no specific +// groups are configured. +func (p *GoogleProvider) populateAllGroups(adminService *admin.Service) func(s *sessions.SessionState) bool { + return func(s *sessions.SessionState) bool { + // Get all groups of the user + groups, err := getUserGroups(adminService, s.Email) + if err != nil { + logger.Errorf("Failed to get user groups for %s: %v", s.Email, err) + s.Groups = []string{} + return true // Allow access even if we can't get groups + } + + // Populate session with all user groups + s.Groups = groups + return true // Always allow access when no specific groups are configured + } +} + // https://developers.google.com/admin-sdk/directory/reference/rest/v1/members/hasMember#authorization-scopes var possibleScopesList = [...]string{ admin.AdminDirectoryGroupMemberReadonlyScope, @@ -269,6 +290,10 @@ func getOauth2TokenSource(ctx context.Context, opts options.GoogleOptions, scope return conf.TokenSource(ctx) } +// getAdminService retrieves an oauth token for the admin api of Google +// AdminEmail has to be an administrative email on the domain that is +// checked. CredentialsFile is the path to a json file containing a Google service +// account credentials. func getAdminService(opts options.GoogleOptions) *admin.Service { ctx := context.Background() var client *http.Client @@ -339,6 +364,38 @@ func getTargetPrincipal(ctx context.Context, opts options.GoogleOptions) (target return targetPrincipal } +// getUserGroups retrieves all groups that a user is a member of using the Google Admin Directory API +func getUserGroups(service *admin.Service, email string) ([]string, error) { + var allGroups []string + var pageToken string + + for { + req := service.Groups.List().UserKey(email).MaxResults(200) + if pageToken != "" { + req = req.PageToken(pageToken) + } + + groupsResp, err := req.Do() + if err != nil { + return nil, fmt.Errorf("failed to list groups for user %s: %v", email, err) + } + + for _, group := range groupsResp.Groups { + if group.Email != "" { + allGroups = append(allGroups, group.Email) + } + } + + // Check if there are more pages + if groupsResp.NextPageToken == "" { + break + } + pageToken = groupsResp.NextPageToken + } + + return allGroups, nil +} + func userInGroup(service *admin.Service, group string, email string) bool { // Use the HasMember API to checking for the user's presence in each group or nested subgroups req := service.Members.HasMember(group, email) diff --git a/providers/google_test.go b/providers/google_test.go index 23bca7ea..dc061203 100644 --- a/providers/google_test.go +++ b/providers/google_test.go @@ -289,3 +289,39 @@ func TestGoogleProvider_userInGroup(t *testing.T) { result = userInGroup(service, "group@example.com", "non-member-out-of-domain@otherexample.com") assert.False(t, result) } + +func TestGoogleProvider_getUserGroups(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/admin/directory/v1/groups" && r.URL.Query().Get("userKey") == "test@example.com" { + response := `{ + "kind": "admin#directory#groups", + "groups": [ + { + "kind": "admin#directory#group", + "id": "1", + "email": "group1@example.com", + "name": "Group 1" + }, + { + "kind": "admin#directory#group", + "id": "2", + "email": "group2@example.com", + "name": "Group 2" + } + ] + }` + fmt.Fprintln(w, response) + } else { + http.NotFound(w, r) + } + })) + defer ts.Close() + + client := &http.Client{} + adminService, err := admin.NewService(context.Background(), option.WithHTTPClient(client), option.WithEndpoint(ts.URL)) + assert.NoError(t, err) + + groups, err := getUserGroups(adminService, "test@example.com") + assert.NoError(t, err) + assert.Equal(t, []string{"group1@example.com", "group2@example.com"}, groups) +}