Repository: dexidp/dex
Pull request #2199 (2021-06-30 12:59:52)

handlePasswordGrant: insert connectorData into OfflineSession
<!--
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

<!-- Describe your changes briefly here. -->

Using dex with the kubernetes oidc client results in the creation
of an Offline Session with empty ConnectorData.
This results in a working login, however, the ldap connector will fail once the client attempts to refresh its token.

This change will insert the ConnectorData from the initial Login
into the OfflineSession, as already done in `handlePasswordLogin`.

#### What this PR does / why we need it

Not doing this results in failures to refresh a token.
I've observed this with the ldap connetor, other connectors might
be affected as well.

The ldap connector will fail with this error message:
`failed to refresh identity: ldap: failed to unmarshal internal data: unexpected end of JSON input`

The `Refresh` method of the ldap connector will first try to unmarshal
the ConnectorData, which was empty, as dex wouldn't
store it in storage after a successful login.
<!--
- 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)"
-->

#### Special notes for your reviewer

While this seems to fix the issue, I'm not sure if it is desired to save the ConnectorData in `handlePasswordGrant` as done in `handlePasswordLogin`.

#### 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
NONE
```


nabokihms: Hello @hensur. Thanks, it looks good. I wonder if we can write a test for this?
hensur: Hey @nabokihms!
I had some time to look into this and came up with a test for this. It won't pass without my changes.
I took a lot of inspiration from the refresh handler tests, as I'm not really familiar with the dex code base :)
nabokihms: Thanks for your contribution @hensur !

Patch:
diff --git a/server/handlers.go b/server/handlers.go
index 4144dd1f..d25cac52 100644
--- a/server/handlers.go
+++ b/server/handlers.go
@@ -1194,9 +1194,10 @@ func (s *Server) handlePasswordGrant(w http.ResponseWriter, r *http.Request, cli
 				return
 			}
 			offlineSessions := storage.OfflineSessions{
-				UserID:  refresh.Claims.UserID,
-				ConnID:  refresh.ConnectorID,
-				Refresh: make(map[string]*storage.RefreshTokenRef),
+				UserID:        refresh.Claims.UserID,
+				ConnID:        refresh.ConnectorID,
+				Refresh:       make(map[string]*storage.RefreshTokenRef),
+				ConnectorData: identity.ConnectorData,
 			}
 			offlineSessions.Refresh[tokenRef.ClientID] = &tokenRef
 
@@ -1226,6 +1227,7 @@ func (s *Server) handlePasswordGrant(w http.ResponseWriter, r *http.Request, cli
 			// Update existing OfflineSession obj with new RefreshTokenRef.
 			if err := s.storage.UpdateOfflineSessions(session.UserID, session.ConnID, func(old storage.OfflineSessions) (storage.OfflineSessions, error) {
 				old.Refresh[tokenRef.ClientID] = &tokenRef
+				old.ConnectorData = identity.ConnectorData
 				return old, nil
 			}); err != nil {
 				s.logger.Errorf("failed to update offline session: %v", err)


Test patch:
diff --git a/connector/mock/connectortest.go b/connector/mock/connectortest.go
index 5089914c..e7ee4386 100644
--- a/connector/mock/connectortest.go
+++ b/connector/mock/connectortest.go
@@ -107,6 +107,7 @@ func (p passwordConnector) Login(ctx context.Context, s connector.Scopes, userna
 			Username:      "Kilgore Trout",
 			Email:         "kilgore@kilgore.trout",
 			EmailVerified: true,
+			ConnectorData: []byte(`{"test": "true"}`),
 		}, true, nil
 	}
 	return identity, false, nil
diff --git a/server/handlers_test.go b/server/handlers_test.go
index a808e5a1..61b3ece6 100644
--- a/server/handlers_test.go
+++ b/server/handlers_test.go
@@ -7,6 +7,8 @@ import (
 	"errors"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
+	"path"
 	"testing"
 	"time"
 
@@ -227,3 +229,75 @@ func TestHandleAuthCode(t *testing.T) {
 		})
 	}
 }
+
+func mockConnectorDataTestStorage(t *testing.T, s storage.Storage) {
+	c := storage.Client{
+		ID:           "test",
+		Secret:       "barfoo",
+		RedirectURIs: []string{"foo://bar.com/", "https://auth.example.com"},
+		Name:         "dex client",
+		LogoURL:      "https://goo.gl/JIyzIC",
+	}
+
+	err := s.CreateClient(c)
+	require.NoError(t, err)
+
+	c1 := storage.Connector{
+		ID:   "test",
+		Type: "mockPassword",
+		Name: "mockPassword",
+		Config: []byte(`{
+"username": "test",
+"password": "test"
+}`),
+	}
+
+	err = s.CreateConnector(c1)
+	require.NoError(t, err)
+}
+
+func TestPasswordConnectorDataNotEmpty(t *testing.T) {
+	t0 := time.Now()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	// Setup a dex server.
+	httpServer, s := newTestServer(ctx, t, func(c *Config) {
+		c.PasswordConnector = "test"
+		c.Now = func() time.Time { return t0 }
+	})
+	defer httpServer.Close()
+
+	mockConnectorDataTestStorage(t, s.storage)
+
+	u, err := url.Parse(s.issuerURL.String())
+	require.NoError(t, err)
+
+	u.Path = path.Join(u.Path, "/token")
+	v := url.Values{}
+	v.Add("scope", "openid offline_access email")
+	v.Add("grant_type", "password")
+	v.Add("username", "test")
+	v.Add("password", "test")
+
+	req, _ := http.NewRequest("POST", u.String(), bytes.NewBufferString(v.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded; param=value")
+	req.SetBasicAuth("test", "barfoo")
+
+	rr := httptest.NewRecorder()
+	s.ServeHTTP(rr, req)
+
+	require.Equal(t, 200, rr.Code)
+
+	// Check that we received expected refresh token
+	var ref struct {
+		Token string `json:"refresh_token"`
+	}
+	err = json.Unmarshal(rr.Body.Bytes(), &ref)
+	require.NoError(t, err)
+
+	newSess, err := s.storage.GetOfflineSessions("0-385-28089-0", "test")
+	require.NoError(t, err)
+	require.Equal(t, `{"test": "true"}`, string(newSess.ConnectorData))
+}