Repository: dexidp/dex
Pull request #2695 (2022-10-04 07:02:13)

Make admin email optional when no service account path is configured
#### Overview

Make admin email optional when no service account path is configured

#### What this PR does / why we need it

This PR reverts #2679 and attempts to fix the original regression reported in #2670 (supposedly introduced in #2530; released in 2.34.0).

The behavior after this PR should be identical to the original behavior IF a service account is configured.

According to #2670 admin email doesn't work when default credentials are used. We need more information on that though for a permanent fix.

@jonkerj do you have anything else to share other than "there was something wrong with default application credentials"?

Also fixes regression reported in #2694 (caused by #2679).

Backport PR also submitted in #2696

#### Special notes for your reviewer

I had no way of testing this change at this time.

#### Does this PR introduce a user-facing change?

<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->

```release-note
Fix regression introduced in #2679
```



Patch:
diff --git a/connector/google/google.go b/connector/google/google.go
index 313858f6..eb9596cd 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
@@ -286,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")
 	}
 
@@ -311,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)))
 }
 


Test patch:
diff --git a/connector/google/google_test.go b/connector/google/google_test.go
index 26241ab6..b0c4f3a2 100644
--- a/connector/google/google_test.go
+++ b/connector/google/google_test.go
@@ -72,22 +72,13 @@ 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"},
+				ClientID:               "testClient",
+				ClientSecret:           "testSecret",
+				RedirectURI:            ts.URL + "/callback",
+				Scopes:                 []string{"openid", "groups"},
+				ServiceAccountFilePath: serviceAccountFilePath,
 			},
 			expectedErr: "requires adminEmail",
 		},
@@ -99,7 +90,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 +101,6 @@ func TestOpen(t *testing.T) {
 				Scopes:                 []string{"openid", "groups"},
 				AdminEmail:             "foo@bar.com",
 				ServiceAccountFilePath: serviceAccountFilePath,
-				Groups:                 []string{"someGroup"},
 			},
 			expectedErr: "",
 		},
@@ -122,7 +111,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 +123,6 @@ func TestOpen(t *testing.T) {
 				Scopes:                 []string{"openid", "groups"},
 				AdminEmail:             "foo@bar.com",
 				ServiceAccountFilePath: serviceAccountFilePath,
-				Groups:                 []string{"someGroup"},
 			},
 			adc:         "/dev/null",
 			expectedErr: "",