Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refresh token rotation #540

Merged
merged 1 commit into from
Aug 16, 2016

Conversation

rsoletob
Copy link
Contributor

@rsoletob rsoletob commented Aug 8, 2016

Added refresh token rotation.

Fixes #519

expectedAud []string
err error
token string
expectedRefreshToken string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you coming up with these values? Can you implement a helper function to do this?

@ericchiang
Copy link
Contributor

A few comments, but overall looks good. Thanks!

@rsoletob rsoletob force-pushed the 519-refresh-token-only-work-once branch from 4d27176 to 804abd1 Compare August 9, 2016 07:43
@rsoletob
Copy link
Contributor Author

rsoletob commented Aug 9, 2016

Thanks for your comments. I see an user could have some refresh tokens for the same client and there is a TODO. Is it correct? I can change it here or in another PR if you think it isn't a valid feature.

@ericchiang
Copy link
Contributor

Probably best for a different PR :)

@rsoletob
Copy link
Contributor Author

Ok @ericchiang, revamped


func (r *refreshTokenRepo) RenewRefreshToken(clientID, userID, oldToken string) (newRefreshToken string, err error) {
// Verify
userID, connectorID, scopes, err := r.verify(nil, clientID, oldToken)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is verify outside the transaction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if dex can't verify the token you save opening a transaction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay sure

@ericchiang
Copy link
Contributor

small nit but other than that tested locally and lgtm

Update refresh token flow to revoke old refresh token and generates a new one.

Fixes dexidp#519
@rsoletob rsoletob force-pushed the 519-refresh-token-only-work-once branch from 804abd1 to c91b37a Compare August 16, 2016 06:05
@ericchiang
Copy link
Contributor

lgtm!

@ericchiang ericchiang merged commit 630bf86 into dexidp:master Aug 16, 2016
@rsoletob rsoletob deleted the 519-refresh-token-only-work-once branch August 31, 2016 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants