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

Oauth2 built-in refresh token #188

Conversation

robsontenorio
Copy link
Contributor

@robsontenorio robsontenorio commented May 18, 2018

@pi0 Another one :)

Automatically refresh the "token" if it expires, because some Oauth providers has a short token TTL. It just intercepts axios requests, check for token expiration and try to refresh it.

Solves #148

@jonasgrosch
Copy link

@robsontenorio Can we get this functionality for local scheme, too?

@robsontenorio
Copy link
Contributor Author

robsontenorio commented May 18, 2018

@jonasgrosch Sorry, this is intended for Oauth2 Scheme. For LOCAL Scheme, it would be very similar.

@pi0 pi0 changed the base branch from dev to next May 21, 2018 15:43
Copy link
Contributor

@ishitatsuyuki ishitatsuyuki left a comment

Choose a reason for hiding this comment

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

There seems to be some mistake on conformance with the OAuth spec.

Also, would you mind adding support of expiring access tokens obtained with the implicit flow (token grant)?

}

// time variables
const tokenExpiresAt = jwtDecode(token).exp * 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not rely on access token being a JWT, but instead use the expires_in response returned along with access_token.


// time variables
const tokenExpiresAt = jwtDecode(token).exp * 1000
const refreshTokenExpiresAt = jwtDecode(refreshToken).exp * 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not try to decode the refresh token. A refresh token is normally not a JWT, and it's the case of Auth0.

// Try to refresh token before processing current request
isRefreshing = true

return $axios.post(this.options.access_token_endpoint,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use async-await and when the refresh fails, please log the user out.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is safe to return a promise.


watchTokenExpiration () {
const { $axios } = this.$auth.ctx.app
let isRefreshing = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use (and wait for) a shared Promise instead of a boolean flag.

@j-evs
Copy link

j-evs commented Jul 24, 2018

Refresh token is a much needed feature. So either this or #208 would be super awesome to have!
@pi0 @ishitatsuyuki 🙏🙏🙏

@robsontenorio
Copy link
Contributor Author

Closing in favor of #208

@HotspurHN
Copy link

any documentation of that feature? my backend returns

{
    jwtToken: {
        { refreshToken: "somevalue", accessToken: "somevalue", expiresIn: "somevalue" }
    },
    response: "Success"
}

how to handle with that refreshToken here?

@robsontenorio
Copy link
Contributor Author

It was not approved/merged, just closed. See #208

@pi0
Copy link
Member

pi0 commented May 23, 2019

@robsontenorio sorry for the late review. This will be hopefully shipped with #325.

@robsontenorio
Copy link
Contributor Author

@pi0 #325 is about LOCAL SCHEME. Will we have auto refresh token for OAUTH SCHEME?

@pi0
Copy link
Member

pi0 commented May 23, 2019

@robsontenorio We may extract shared functionalities and share them between oauth and local.

@MathiasCiarlo
Copy link
Collaborator

MathiasCiarlo commented May 24, 2019

@pi0 Could we do this and include it in 4.6.0? Or do you think it's too much work to get it done in time?

@pi0
Copy link
Member

pi0 commented May 24, 2019

@MathiasCiarlo We need some refactors but don't want to block shipping other fixes for refresh support. Let's see what happens but if not included in 4.6, 4.7 or 5.0 will be worked right after that.

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.

7 participants