From 19b3aab323447500d937bf8d0543630ec95c1eba Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Tue, 4 Oct 2022 08:49:14 +0200 Subject: [PATCH 1/2] Revert "fix: check for no serviceAccountFilePath and no email (#2679)" This reverts commit 49477729ce24448c2895ec8c98f2c61c646de884. Signed-off-by: Mark Sagi-Kazar --- connector/google/google.go | 11 ++++------- connector/google/google_test.go | 14 -------------- 2 files changed, 4 insertions(+), 21 deletions(-) diff --git a/connector/google/google.go b/connector/google/google.go index 313858f6..72cc6a18 100644 --- a/connector/google/google.go +++ b/connector/google/google.go @@ -71,13 +71,10 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e scopes = append(scopes, "profile", "email") } - var srv *admin.Service - if len(c.Groups) > 0 { - srv, err = createDirectoryService(c.ServiceAccountFilePath, c.AdminEmail, logger) - if err != nil { - cancel() - return nil, fmt.Errorf("could not create directory service: %v", err) - } + srv, err := createDirectoryService(c.ServiceAccountFilePath, c.AdminEmail, logger) + if err != nil { + cancel() + return nil, fmt.Errorf("could not create directory service: %v", err) } clientID := c.ClientID diff --git a/connector/google/google_test.go b/connector/google/google_test.go index 26241ab6..5cecbec9 100644 --- a/connector/google/google_test.go +++ b/connector/google/google_test.go @@ -72,22 +72,12 @@ func TestOpen(t *testing.T) { assert.Nil(t, err) for name, reference := range map[string]testCase{ - "not_requesting_groups": { - config: &Config{ - ClientID: "testClient", - ClientSecret: "testSecret", - RedirectURI: ts.URL + "/callback", - Scopes: []string{"openid"}, - }, - expectedErr: "", - }, "missing_admin_email": { config: &Config{ ClientID: "testClient", ClientSecret: "testSecret", RedirectURI: ts.URL + "/callback", Scopes: []string{"openid", "groups"}, - Groups: []string{"someGroup"}, }, expectedErr: "requires adminEmail", }, @@ -99,7 +89,6 @@ func TestOpen(t *testing.T) { Scopes: []string{"openid", "groups"}, AdminEmail: "foo@bar.com", ServiceAccountFilePath: "not_found.json", - Groups: []string{"someGroup"}, }, expectedErr: "error reading credentials", }, @@ -111,7 +100,6 @@ func TestOpen(t *testing.T) { Scopes: []string{"openid", "groups"}, AdminEmail: "foo@bar.com", ServiceAccountFilePath: serviceAccountFilePath, - Groups: []string{"someGroup"}, }, expectedErr: "", }, @@ -122,7 +110,6 @@ func TestOpen(t *testing.T) { RedirectURI: ts.URL + "/callback", Scopes: []string{"openid", "groups"}, AdminEmail: "foo@bar.com", - Groups: []string{"someGroup"}, }, adc: serviceAccountFilePath, expectedErr: "", @@ -135,7 +122,6 @@ func TestOpen(t *testing.T) { Scopes: []string{"openid", "groups"}, AdminEmail: "foo@bar.com", ServiceAccountFilePath: serviceAccountFilePath, - Groups: []string{"someGroup"}, }, adc: "/dev/null", expectedErr: "", From 261adee26b5778f61bb2deb0d3c8ec5242f6ee80 Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Tue, 4 Oct 2022 08:55:57 +0200 Subject: [PATCH 2/2] fix(connector/google): make admin email optional for default creds Signed-off-by: Mark Sagi-Kazar --- connector/google/google.go | 11 +++++++++-- connector/google/google_test.go | 9 +++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/connector/google/google.go b/connector/google/google.go index 72cc6a18..eb9596cd 100644 --- a/connector/google/google.go +++ b/connector/google/google.go @@ -283,7 +283,9 @@ func (c *googleConnector) getGroups(email string, fetchTransitiveGroupMembership // the google admin api. If no serviceAccountFilePath is defined, the application default credential // is used. func createDirectoryService(serviceAccountFilePath, email string, logger log.Logger) (*admin.Service, error) { - if email == "" { + // We know impersonation is required when using a service account credential + // TODO: or is it? + if email == "" && serviceAccountFilePath != "" { return nil, fmt.Errorf("directory service requires adminEmail") } @@ -308,7 +310,12 @@ func createDirectoryService(serviceAccountFilePath, email string, logger log.Log if err != nil { return nil, fmt.Errorf("unable to parse credentials to config: %v", err) } - config.Subject = email + + // Only attempt impersonation when there is a user configured + if email != "" { + config.Subject = email + } + return admin.NewService(ctx, option.WithHTTPClient(config.Client(ctx))) } diff --git a/connector/google/google_test.go b/connector/google/google_test.go index 5cecbec9..b0c4f3a2 100644 --- a/connector/google/google_test.go +++ b/connector/google/google_test.go @@ -74,10 +74,11 @@ func TestOpen(t *testing.T) { for name, reference := range map[string]testCase{ "missing_admin_email": { config: &Config{ - ClientID: "testClient", - ClientSecret: "testSecret", - RedirectURI: ts.URL + "/callback", - Scopes: []string{"openid", "groups"}, + ClientID: "testClient", + ClientSecret: "testSecret", + RedirectURI: ts.URL + "/callback", + Scopes: []string{"openid", "groups"}, + ServiceAccountFilePath: serviceAccountFilePath, }, expectedErr: "requires adminEmail", },