Implement refreshing with Google #1180

JoelSpeed · 2018-01-29T21:34:07Z

Fixes: #863

I've read through the document provided in this comment by @ericchiang and implemented the changes he describes.

I've been testing it with the memory storage and connecting to Google so far and everything seems to be working (Added lots of Prints while debugging).

Have had two clients set up, logged the same user into A, then into B (updating the refresh token since A's is now invalid), then refreshed client A, which used the token retrieved by client B to refresh.

I believe this should now allow the creation of a specific Google connector, in turn allowing someone to implement retrieval of groups using their Admin Directory API

ericchiang · 2018-02-01T19:29:40Z

This is a big PR so I just have some overall questions before I dive into a review.

How does the migration work? If a user already has a refresh token in the system, then they upgrade dex, what happens to the connector data associated with that refresh token?

JoelSpeed · 2018-02-01T20:22:37Z

At present, they lose it I believe.

I had a look through the code to try and mitigate that but couldn't see anything obvious.

I'm guessing if there is a solution it would be in the storage section of the codebase. Do the SQL/K8s/Etcd storage backends have migration scripts that can be amended?

JoelSpeed · 2018-02-01T20:36:41Z

server/handlers.go

+	} else {
+		connectorData = session.ConnectorData
+	}
+


I guess to allow migration some code could be added here, checking if the ConnectorData is present, if not check refresh object for it. But this means putting the ConnectorData field back in the struct and would probably mean this migration code would be there forever.

I guess it depends how important you think maintaining existing refresh tokens is

JoelSpeed · 2018-02-05T21:14:33Z

Did a little refactoring and built an image to make it easier for anyone wanting to test these changes: quay.io/joelspeed/dex:refresh-tokens-1

JoelSpeed · 2018-02-15T10:11:37Z

Have just tested this with an existing SQL DB and realised the migrations didn't work quite as I thought they did. I've fixed this now and migrations from existing installations are now working. New image quay.io/joelspeed/dex:refresh-tokens-2

JoelSpeed · 2018-02-15T11:11:24Z

There was also an error in the updater function for offline sessions in the SQL connector, this is now fixed. quay.io/joelspeed/dex:refresh-tokens-3

jhohertz · 2018-05-16T15:13:06Z

I'm using this now in conjunction with #1185, would love to see both mainlined. Let me know if I can help or test in any way.

marco-m · 2018-09-26T21:03:51Z

@ericchiang did you have a chance to review this PR ?

JoelSpeed · 2019-01-28T13:19:07Z

Rebased onto latest master

JoelSpeed · 2019-02-20T14:43:51Z

@ericchiang @srenatus Just been talking about this on the K8s office hours, any chance either of you have time to review?

srenatus · 2019-02-21T15:11:00Z

Count me out for the next few months, please: I'm on parental leave. :)

…

On Wed, 20 Feb 2019, 15:43 Joel Speed, ***@***.***> wrote: @ericchiang <https://github.com/ericchiang> @srenatus <https://github.com/srenatus> Just been talking about this on the K8s office hours, any chance either of you have time to review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1180 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA1I7t2TNEqaU-WzHG3jW7icUg2IgmnZks5vPV8pgaJpZM4RxZAe> .

ericchiang · 2019-02-22T19:07:40Z

Sorry, context switching back to this PR.

Did the migration strategy ever get worked out? I'm particularly interested because dex is horizontally scalable, so we have to tolerate multiple versions of dex running at the same time during a live migration. What happens with the ConnectorData in that case?

tsuna · 2019-04-05T17:30:22Z

Is this PR still active somehow or is this effort kind of abandoned at this point? Would love to see this merged!

JoelSpeed · 2019-04-12T08:44:54Z

I dropped the ball on this a bit, but will try and take a look at #1180 (comment) and respond within the next week, sorry for the delay

JoelSpeed · 2019-04-23T10:10:22Z

@ericchiang I've reverted all of the changes to remove the old connector data and added some migration code in the final commit, WDYT?

I'll squash all the reverts into their original commits to clean up the history if you are happy with this migration approach

connector/oidc/oidc.go

ericchiang

This should be good to merge soon.

One point though. Dex may be running in a setup where there are multiple versions of dex. If we're running vN, then upgrade to vN+1, the later version will be writing fields that vN doesn't know about (and possibly overwritting due to marshaling quirks).

Can we introduce the ConnectorData in version vN+1, but don't store any data in it until vN+2? That way by deployments run vN+2 any other versions of dex will already understand how to interpret ConnectorData.

ericchiang · 2019-05-12T16:54:52Z

server/handlers.go

+		}
+	} else if len(refresh.ConnectorData) > 0 {
+		// Use the old connector data if it exists, should be deleted once used
+		connectorData = session.ConnectorData


Shouldn't this be refresh.ConnectorData?

ericchiang · 2019-05-12T16:55:37Z

connector/oidc/oidc.go

@@ -224,11 +248,7 @@ func (c *oidcConnector) HandleCallback(s connector.Scopes, r *http.Request) (ide
 		Username:      claims.Username,
 		Email:         claims.Email,
 		EmailVerified: claims.EmailVerified,
+		ConnectorData: []byte(token.RefreshToken),


nit: this should probably be a marshaled struct like the existing ConnectorData, in case we want to add additional data later.

I'm not sure I follow. Isn't token.RefreshToken opaque (probably random) data? What struct would this be then? ... 💡 Oh I see.

type cd struct { refreshToken []byte } // ... identity = connector.Identity{ UserID: idToken.Subject, Username: claims.Username, Email: claims.Email, EmailVerified: claims.EmailVerified, ConnectorData: cd{refreshToken: []byte(token.RefreshToken)}, }

....yeah I think some kind of struct would be better here. 👍

ericchiang · 2019-05-12T17:01:10Z

server/handlers.go

@@ -485,6 +485,41 @@ func (s *Server) finalizeLogin(identity connector.Identity, authReq storage.Auth
 	s.logger.Infof("login successful: connector %q, username=%q, email=%q, groups=%q",
 		authReq.ConnectorID, claims.Username, email, claims.Groups)

+	if _, ok := conn.(connector.RefreshConnector); ok {


nit: Maybe avoid this indenting?

returnURL := path.Join(s.issuerURL.Path, "/approval") + "?req=" + authReq.ID refreshConn, ok := conn.(connector.RefreshConnector) if !ok { return returnURL, nil } session, err := s.storage.GetOfflineSessions(identity.UserID, authReq.ConnectorID) if err != nil { if err != storage.ErrNotFound { // ... } if err := s.storage.CreateOfflineSessions(...) { // ... } return returnURL, nil } if err := s.storage.UpdateOfflineSessions( ... ); err != nil { // ... } return returnURL, nil

ericchiang · 2019-05-12T17:05:49Z

Also please squash 😃

dbyron0 · 2019-07-03T15:57:47Z

@JoelSpeed is this PR still alive? Would love to see it go in.

JoelSpeed · 2019-07-03T16:01:24Z

@JoelSpeed is this PR still alive? Would love to see it go in.

Hey, yeah it is, sorry for the delay, hoping to get some time to work on it next week, it's been pretty low priority for me unfortunately

Can we introduce the ConnectorData in version vN+1, but don't store any data in it until vN+2? That way by deployments run vN+2 any other versions of dex will already understand how to interpret ConnectorData.

@ericchiang I'll try and create a PR to do this next week and we can go from there. Is it worth having the fallback logic so that it tries to load from the future ConnectorData if the current isn't set? Then switch that in the next version. Then theoretically both versions should be able to read the data?

srenatus

This generally looks decent, I'll follow Eric in the struct comment, though, I think.

And I haven't yet had a chance to play with this.

In general, I think reviving it would be great. 👍 💯

srenatus · 2019-05-10T18:56:00Z

connector/oidc/oidc.go

+	}
+	token, err := c.oauth2Config.TokenSource(ctx, t).Token()
+	if err != nil {
+		return identity, fmt.Errorf("oidc: failed to get token: %v", err)


Can we make this say "... refresh token..." to differentiate it from the error logged above?

srenatus · 2019-09-04T10:02:00Z

connector/oidc/oidc.go

@@ -224,11 +248,7 @@ func (c *oidcConnector) HandleCallback(s connector.Scopes, r *http.Request) (ide
 		Username:      claims.Username,
 		Email:         claims.Email,
 		EmailVerified: claims.EmailVerified,
+		ConnectorData: []byte(token.RefreshToken),


I'm not sure I follow. Isn't token.RefreshToken opaque (probably random) data? What struct would this be then? ... 💡 Oh I see.

type cd struct { refreshToken []byte } // ... identity = connector.Identity{ UserID: idToken.Subject, Username: claims.Username, Email: claims.Email, EmailVerified: claims.EmailVerified, ConnectorData: cd{refreshToken: []byte(token.RefreshToken)}, }

....yeah I think some kind of struct would be better here. 👍

JoelSpeed · 2019-09-25T20:36:43Z

@srenatus and anyone else interested, I think I've fixed this up based on the comments provided so far, it's also been rebased on master so should now be conflict free!

I didn't realise quite what the migration mechanism was. Have understood it now.

This reverts commit 27f3351.

This reverts commit 9c7ceabe8aebf6c740c237c5e76c21397179f901.

This reverts commit 228bdc324877bf67ecdd434503b9c1b25d8e7d28.

JoelSpeed · 2019-11-19T15:46:11Z

@quantonganh I've rebased as requested, should be up to date with master now

@bonifaido Do you have any time to review this? I think I've addressed all of the comments made so far

I'm keen to get this done by the end of the year, I'm changing role and I'm not sure I'll have opportunity to test changes as easily from January

bonifaido

Great work! LGTM, I think it's time to let this in ;)

JoelSpeed · 2019-11-19T16:40:30Z

Thanks @bonifaido and everyone else who has reviewed or had intrigue in this over the last couple of years! 🎉

JoelSpeed commented Feb 1, 2018

View reviewed changes

JoelSpeed mentioned this pull request Feb 6, 2018

Fetch groups in a Google Connector #1185

Merged

colemickens mentioned this pull request Jun 26, 2018

Support the implicit grant flow (w/ response_mode=post_form) #1256

Open

gabrielgrant mentioned this pull request Jul 3, 2018

Allow authentication with LDAP in Pachyderm pachyderm/pachyderm#2428

Open

JoelSpeed force-pushed the refresh-tokens branch from 26a083f to 540969b Compare January 28, 2019 13:18

JoelSpeed force-pushed the refresh-tokens branch from 540969b to af1d36f Compare April 23, 2019 10:08

klarose reviewed May 10, 2019

View reviewed changes

connector/oidc/oidc.go Outdated Show resolved Hide resolved

ericchiang reviewed May 12, 2019

View reviewed changes

ericchiang mentioned this pull request May 12, 2019

Add option to enable groups for oidc connectors #1434

Merged

srenatus reviewed Sep 4, 2019

View reviewed changes

justin-slowik added a commit to justin-slowik/dex that referenced this pull request Sep 23, 2019

Implement upstream refresh token handling. Based on dexidp#1180

4a493c5

JoelSpeed force-pushed the refresh-tokens branch from 5b84fc4 to dd3e5b3 Compare September 25, 2019 20:34

JoelSpeed added 23 commits November 19, 2019 15:43

Update SQL storage backend

7fc3f23

Update conformance

c789c58

Fix ETCD storage backend

c54f165

Update Kubernetes storage backend

7a76c76

Fix conformance tests

b9b315d

Fix SQL storage

80995df

Build opts based on scope

4076eed

Remove duplicate code

433bb2a

Fix migration in SQL connector

d389098

I didn't realise quite what the migration mechanism was. Have understood it now.

Fix SQL updater func

fea048b

Revert "Remove connectordata from other structs"

176ba70

This reverts commit 27f3351.

Revert "Update SQL storage backend"

9ce4393

Revert "Update conformance"

41b7c85

This reverts commit 9c7ceabe8aebf6c740c237c5e76c21397179f901.

Revert "Fix ETCD storage backend"

236b25b

Revert "Update Kubernetes storage backend"

45a40a1

This reverts commit 228bdc324877bf67ecdd434503b9c1b25d8e7d28.

Use old ConnectorData before session.ConnectorData

19ad7da

Fix Refresh comment

8b344fe

Identify error as failure to retrieve refresh token

f607708

Use a struct for connector data within OIDC connector

77fcf9a

Unindent session updates on finalizeLogin

d909507

Fix migration of old connector data

c4e96dd

Remove defaulting from connector_data column

c782ac8

OIDC: Rename refreshToken to RefreshToken

3156553

JoelSpeed force-pushed the refresh-tokens branch from bed6f4a to 3156553 Compare November 19, 2019 15:43

bonifaido approved these changes Nov 19, 2019

View reviewed changes

bonifaido merged commit b1e98d8 into dexidp:master Nov 19, 2019

dskatz mentioned this pull request Dec 4, 2019

Propagate groups from upstream OIDC provider #1065

Open

cisco-abrandel mentioned this pull request May 13, 2022

Remove stale caveat that indicates changed upstream claims won't be sent downstream from dex dexidp/website#113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement refreshing with Google #1180

Implement refreshing with Google #1180

JoelSpeed commented Jan 29, 2018

ericchiang commented Feb 1, 2018

JoelSpeed commented Feb 1, 2018

JoelSpeed Feb 1, 2018

JoelSpeed commented Feb 5, 2018

JoelSpeed commented Feb 15, 2018

JoelSpeed commented Feb 15, 2018

jhohertz commented May 16, 2018

marco-m commented Sep 26, 2018

JoelSpeed commented Jan 28, 2019

JoelSpeed commented Feb 20, 2019

srenatus commented Feb 21, 2019 via email

ericchiang commented Feb 22, 2019

tsuna commented Apr 5, 2019

JoelSpeed commented Apr 12, 2019

JoelSpeed commented Apr 23, 2019

ericchiang left a comment

ericchiang May 12, 2019

ericchiang May 12, 2019

srenatus Sep 4, 2019

ericchiang May 12, 2019

ericchiang commented May 12, 2019

dbyron0 commented Jul 3, 2019

JoelSpeed commented Jul 3, 2019

srenatus left a comment

srenatus May 10, 2019

srenatus Sep 4, 2019

JoelSpeed commented Sep 25, 2019

JoelSpeed commented Nov 19, 2019

bonifaido left a comment

JoelSpeed commented Nov 19, 2019

Implement refreshing with Google #1180

Implement refreshing with Google #1180

Conversation

JoelSpeed commented Jan 29, 2018

ericchiang commented Feb 1, 2018

JoelSpeed commented Feb 1, 2018

JoelSpeed Feb 1, 2018

Choose a reason for hiding this comment

JoelSpeed commented Feb 5, 2018

JoelSpeed commented Feb 15, 2018

JoelSpeed commented Feb 15, 2018

jhohertz commented May 16, 2018

marco-m commented Sep 26, 2018

JoelSpeed commented Jan 28, 2019

JoelSpeed commented Feb 20, 2019

srenatus commented Feb 21, 2019 via email

ericchiang commented Feb 22, 2019

tsuna commented Apr 5, 2019

JoelSpeed commented Apr 12, 2019

JoelSpeed commented Apr 23, 2019

ericchiang left a comment

Choose a reason for hiding this comment

ericchiang May 12, 2019

Choose a reason for hiding this comment

ericchiang May 12, 2019

Choose a reason for hiding this comment

srenatus Sep 4, 2019

Choose a reason for hiding this comment

ericchiang May 12, 2019

Choose a reason for hiding this comment

ericchiang commented May 12, 2019

dbyron0 commented Jul 3, 2019

JoelSpeed commented Jul 3, 2019

srenatus left a comment

Choose a reason for hiding this comment

srenatus May 10, 2019

Choose a reason for hiding this comment

srenatus Sep 4, 2019

Choose a reason for hiding this comment

JoelSpeed commented Sep 25, 2019

JoelSpeed commented Nov 19, 2019

bonifaido left a comment

Choose a reason for hiding this comment

JoelSpeed commented Nov 19, 2019