Repository: dexidp/dex
Pull request #2522 (2022-05-20 04:19:02)

OIDC connector: Support cases where there is no id_token when using a refresh_token grant
Signed-off-by: Anthony Brandelli <abrandel@cisco.com>

<!--
Thank you for sending a pull request! Here are some tips for contributors:

1. Fill the description template below.
2. Sign a DCO (if you haven't already signed it).
3. Include appropriate tests (if necessary). Make sure that all CI checks passed.
4. If the Pull Request is a work in progress, make use of GitHub's "Draft PR" feature and mark it as such.
-->

#### Overview

I broke https://github.com/dexidp/dex/pull/2505 with a bad rebase, this is a PR off a fixed branch, sorry for the confusion @nabokihms 

<!-- Describe your changes briefly here. -->
Allows for dex to function with IDPs that do not return an ID token in the reply when using a refresh_token grant.

#### What this PR does / why we need it

<!--
- Please state in detail why we need this PR and what it solves.
- If your PR closes some of the existing issues, please add links to them here.
  Mentioned issues will be automatically closed.
  Usage: "Closes #<issue number>", or "Closes (paste link of issue)"
-->

Although this seems to be rare, the OIDC spec does not require that an id_token be present in the reply when using a refresh_token grant. Additional info in https://github.com/dexidp/dex/issues/2498.

This is a toggle in PingFed, so there is at least one common IDP out there where there won't always be a refresh token.

Closes https://github.com/dexidp/dex/issues/2498

#### Special notes for your reviewer

Note that if a user has an IDP where ID tokens aren't present when using a refresh_token grant, AND also chooses not to enable the userinfo endpoint, this will choke. The claims have to come from somewhere, so I would say this really is just an unsupported configuration should someone choose to try this. The OIDC spec says that userinfo must always contain the sub additionally.

I tried to keep behaviour as close as possible to current. Although its extra code, and probably not needed, I chose to prefer the sub from the ID token if its present, over assigning the one from the userinfo.

#### 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
OIDC connector: Support cases where there is no id_token when using a refresh_token grant
```



Patch:
diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go
index 5e995d1b..b4e67799 100644
--- a/connector/oidc/oidc.go
+++ b/connector/oidc/oidc.go
@@ -226,6 +226,13 @@ func (e *oauth2Error) Error() string {
 	return e.error + ": " + e.errorDescription
 }
 
+type caller uint
+
+const (
+	createCaller caller = iota
+	refreshCaller
+)
+
 func (c *oidcConnector) HandleCallback(s connector.Scopes, r *http.Request) (identity connector.Identity, err error) {
 	q := r.URL.Query()
 	if errType := q.Get("error"); errType != "" {
@@ -235,8 +242,7 @@ func (c *oidcConnector) HandleCallback(s connector.Scopes, r *http.Request) (ide
 	if err != nil {
 		return identity, fmt.Errorf("oidc: failed to get token: %v", err)
 	}
-
-	return c.createIdentity(r.Context(), identity, token)
+	return c.createIdentity(r.Context(), identity, token, createCaller)
 }
 
 // Refresh is used to refresh a session with the refresh token provided by the IdP
@@ -255,23 +261,25 @@ func (c *oidcConnector) Refresh(ctx context.Context, s connector.Scopes, identit
 	if err != nil {
 		return identity, fmt.Errorf("oidc: failed to get refresh token: %v", err)
 	}
-
-	return c.createIdentity(ctx, identity, token)
+	return c.createIdentity(ctx, identity, token, refreshCaller)
 }
 
-func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.Identity, token *oauth2.Token) (connector.Identity, error) {
+func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.Identity, token *oauth2.Token, caller caller) (connector.Identity, error) {
+	var claims map[string]interface{}
+
 	rawIDToken, ok := token.Extra("id_token").(string)
-	if !ok {
-		return identity, errors.New("oidc: no id_token in token response")
-	}
-	idToken, err := c.verifier.Verify(ctx, rawIDToken)
-	if err != nil {
-		return identity, fmt.Errorf("oidc: failed to verify ID Token: %v", err)
-	}
+	if ok {
+		idToken, err := c.verifier.Verify(ctx, rawIDToken)
+		if err != nil {
+			return identity, fmt.Errorf("oidc: failed to verify ID Token: %v", err)
+		}
 
-	var claims map[string]interface{}
-	if err := idToken.Claims(&claims); err != nil {
-		return identity, fmt.Errorf("oidc: failed to decode claims: %v", err)
+		if err := idToken.Claims(&claims); err != nil {
+			return identity, fmt.Errorf("oidc: failed to decode claims: %v", err)
+		}
+	} else if caller != refreshCaller {
+		// ID tokens aren't mandatory in the reply when using a refresh_token grant
+		return identity, errors.New("oidc: no id_token in token response")
 	}
 
 	// We immediately want to run getUserInfo if configured before we validate the claims
@@ -285,6 +293,12 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
 		}
 	}
 
+	const subjectClaimKey = "sub"
+	subject, found := claims[subjectClaimKey].(string)
+	if !found {
+		return identity, fmt.Errorf("missing \"%s\" claim", subjectClaimKey)
+	}
+
 	userNameKey := "name"
 	if c.userNameKey != "" {
 		userNameKey = c.userNameKey
@@ -358,7 +372,7 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
 	}
 
 	identity = connector.Identity{
-		UserID:            idToken.Subject,
+		UserID:            subject,
 		Username:          name,
 		PreferredUsername: preferredUsername,
 		Email:             email,


Test patch:
diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go
index 3038cebc..d8b30b39 100644
--- a/connector/oidc/oidc_test.go
+++ b/connector/oidc/oidc_test.go
@@ -275,7 +275,8 @@ func TestHandleCallback(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			testServer, err := setupServer(tc.token)
+			idTokenDesired := true
+			testServer, err := setupServer(tc.token, idTokenDesired)
 			if err != nil {
 				t.Fatal("failed to setup test server", err)
 			}
@@ -331,7 +332,87 @@ func TestHandleCallback(t *testing.T) {
 	}
 }
 
-func setupServer(tok map[string]interface{}) (*httptest.Server, error) {
+func TestRefresh(t *testing.T) {
+	t.Helper()
+
+	tests := []struct {
+		name           string
+		expectUserID   string
+		expectUserName string
+		idTokenDesired bool
+		token          map[string]interface{}
+	}{
+		{
+			name:           "IDTokenOnRefresh",
+			expectUserID:   "subvalue",
+			expectUserName: "namevalue",
+			idTokenDesired: true,
+			token: map[string]interface{}{
+				"sub":  "subvalue",
+				"name": "namevalue",
+			},
+		},
+		{
+			name:           "NoIDTokenOnRefresh",
+			expectUserID:   "subvalue",
+			expectUserName: "namevalue",
+			idTokenDesired: false,
+			token: map[string]interface{}{
+				"sub":  "subvalue",
+				"name": "namevalue",
+			},
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			testServer, err := setupServer(tc.token, tc.idTokenDesired)
+			if err != nil {
+				t.Fatal("failed to setup test server", err)
+			}
+			defer testServer.Close()
+
+			scopes := []string{"openid", "offline_access"}
+			serverURL := testServer.URL
+			config := Config{
+				Issuer:       serverURL,
+				ClientID:     "clientID",
+				ClientSecret: "clientSecret",
+				Scopes:       scopes,
+				RedirectURI:  fmt.Sprintf("%s/callback", serverURL),
+				GetUserInfo:  true,
+			}
+
+			conn, err := newConnector(config)
+			if err != nil {
+				t.Fatal("failed to create new connector", err)
+			}
+
+			req, err := newRequestWithAuthCode(testServer.URL, "someCode")
+			if err != nil {
+				t.Fatal("failed to create request", err)
+			}
+
+			refreshTokenStr := "{\"RefreshToken\":\"asdf\"}"
+			refreshToken := []byte(refreshTokenStr)
+
+			identity := connector.Identity{
+				UserID:        tc.expectUserID,
+				Username:      tc.expectUserName,
+				ConnectorData: refreshToken,
+			}
+
+			refreshIdentity, err := conn.Refresh(req.Context(), connector.Scopes{OfflineAccess: true}, identity)
+			if err != nil {
+				t.Fatal("Refresh failed", err)
+			}
+
+			expectEquals(t, refreshIdentity.UserID, tc.expectUserID)
+			expectEquals(t, refreshIdentity.Username, tc.expectUserName)
+		})
+	}
+}
+
+func setupServer(tok map[string]interface{}, idTokenDesired bool) (*httptest.Server, error) {
 	key, err := rsa.GenerateKey(rand.Reader, 1024)
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate rsa key: %v", err)
@@ -368,11 +449,23 @@ func setupServer(tok map[string]interface{}) (*httptest.Server, error) {
 		}
 
 		w.Header().Add("Content-Type", "application/json")
-		json.NewEncoder(w).Encode(&map[string]string{
-			"access_token": token,
-			"id_token":     token,
-			"token_type":   "Bearer",
-		})
+		if idTokenDesired {
+			json.NewEncoder(w).Encode(&map[string]string{
+				"access_token": token,
+				"id_token":     token,
+				"token_type":   "Bearer",
+			})
+		} else {
+			json.NewEncoder(w).Encode(&map[string]string{
+				"access_token": token,
+				"token_type":   "Bearer",
+			})
+		}
+	})
+
+	mux.HandleFunc("/userinfo", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Add("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(tok)
 	})
 
 	mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {