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

feat: refresh support #361

Merged
merged 131 commits into from
Mar 15, 2020
Merged

feat: refresh support #361

merged 131 commits into from
Mar 15, 2020

Conversation

pi0
Copy link
Member

@pi0 pi0 commented May 25, 2019

Based on #325 (@JoaoPedroAS51), #208 (@KonoMaxi) -- so main credits backs to authors. This PR is preparing to merge into upstream.

This closes #261, closes #398 and closes #67

Note

Refresh support will be released in v5. If you want to use it in the meantime, use the package @nuxtjs/auth-next. Please, check the updated docs for dev version https://dev.auth.nuxtjs.org/

TODO

Co-authored-by: João Pedro Antunes Silva <[email protected]>
@pi0 pi0 added the WIP label May 25, 2019
@MathiasCiarlo
Copy link
Collaborator

Some thoughts on auto refresh:
Should this not be disabled by default? I think auto refresh is a potential security risk. The user should be logged out when the refresh token expires. Personally I think auto refresh should not be an option at all, but if you are going to include it - I think it at least should be disabled by default.

@pi0
Copy link
Member Author

pi0 commented May 26, 2019

@MathiasCiarlo If logging out the user when token expires what's the purpose of the refrash token? Can you please explain more?

In current implementation refrash is a new auth scheme so not enabled by default. It will be probably converted to an optional feature of local and oauth2 but when enabled, it's purpose is auto-refreshing token before being expired.

@MathiasCiarlo
Copy link
Collaborator

MathiasCiarlo commented May 26, 2019

@pi0 I don't mean the user should be logged out when the access token expires, but if no request/activity has been made before the refresh token expires, I think the user should be logged out.

Lets say the access token expires after 5 minutes, and the refresh token expires after 30 minutes. If a user logs in, and leaves his or her computer for a really long coffee break, the session should expire after 30 minutes of inactivity. The user can keep the session alive by refreshing the token before the 30 minute mark.

I have been using @robsontenorio's fork, where the token is only refreshed when the user performs a request, thus being active. I think this is a safer approach, since it only keeps active users logged in.

So if I understand you correctly - the new refresh scheme is only temporary (and only works for local scheme), and will not be a scheme in the future? The goal is built in refresh functionality for both local and oauth2?

@pi0
Copy link
Member Author

pi0 commented May 26, 2019

The goal is built in refresh functionality for both local and oauth2?

Yes exactly


Regarding auto-refresh timer, totally makes sense to be disabled by default. Does current error interceptor looks good/enough to you?

Copy link
Collaborator

@MathiasCiarlo MathiasCiarlo left a comment

Choose a reason for hiding this comment

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

Some more questions 🙂

lib/schemes/refresh.js Outdated Show resolved Hide resolved
lib/schemes/refresh.js Outdated Show resolved Hide resolved
lib/schemes/refresh.js Outdated Show resolved Hide resolved
lib/schemes/refresh.js Outdated Show resolved Hide resolved
@JoaoPedroAS51
Copy link
Collaborator

I think it's because the token is refreshed when it has expired, not when invalidated.

@iteamaster89
Copy link

So, if I get 401 because of invalid token it won’t refresh, right?

@JoaoPedroAS51
Copy link
Collaborator

Right. We don't intercept 401 anymore.

@iteamaster89
Copy link

Got it. Thanks! Have a good day:)

@JoaoPedroAS51
Copy link
Collaborator

Thanks! You too! :)

@JSeifBY
Copy link

JSeifBY commented May 5, 2020

Hello @JoaoPedroAS51 , so to refresh token after a 401 we need to do it independently from auth module ?

@JoaoPedroAS51
Copy link
Collaborator

Hi @JSeifBY! Yes, if you want to refresh after a 401 you need to do it yourself. But if you have a 401 because of an expired token, means that our system is not working well. Then please let me know. :)

We adopted the practice of refresh tokens based on the token expiration. We intercept requests and check for token expiration. If the token has expired then we refresh. The original request will await for refresh request.

A 401 error means that you are unauthorized to access the target resource, but it doesn't mean that the token has expired. That's why we base the refresh on token expiration.

@JSeifBY
Copy link

JSeifBY commented May 5, 2020

Many thanks for those clarifications @JoaoPedroAS51 🙏

@thunderwin
Copy link

I can't wait to use V5....

@ismailonly
Copy link

ismailonly commented May 12, 2020

Hello @JoaoPedroAS51 , My tokens are not being saved in the Local Storage / Cookie. In Local storage I see auth._refresh_token.local: false and auth._token.local: Bearer undefined. The server correctly returns the 'access' and 'refresh' tokens - and I have named these in the auth options.

@JoaoPedroAS51
Copy link
Collaborator

Hi @ismailonly! What version are you using?

@JoaoPedroAS51
Copy link
Collaborator

@ismailonly Can you also show me your auth config? :)

@ismailonly
Copy link

version: 4.9.1

auth: {
strategies: {
local: {
scheme: 'refresh',
autoRefresh: {
enable: true
},
clientId: false,
grantType: false,
token: {
property: 'access',
expiresIn: 'exp'
},
refreshToken: {
property: 'refresh',
expiresIn: 'exp'
},
user: 'user',
endpoints: {
login: { url: 'http://127.0.0.1:8000/api/token/', method: 'post' },
refresh: { url: 'http://127.0.0.1:8000/api/token/refresh/', method: 'post' },
user: { url: 'http://127.0.0.1:8000/api/userinfo/', method: 'get' },
logout: { url: 'http://127.0.0.1:8000/api/logout/', method: 'get'}
}
}
}
}

@JoaoPedroAS51
Copy link
Collaborator

@ismailonly Thank you! :)

So, refresh support wasn't released yet. It will be released in v5, but you can use the dev version in the meantime. We published a new package for dev version nuxtjs/auth-next

Please, check the updated docs for dev version, because we made a lot of changes https://dev.auth.nuxtjs.org/

@ismailonly
Copy link

Thanks for the quick resonse @JoaoPedroAS51

I will check it out.

@ismailonly
Copy link

@JoaoPedroAS51 is the 'expiresIn' option removed in v5?

@JoaoPedroAS51
Copy link
Collaborator

@ismailonly Yes, it was removed. Now you must set maxAge of both tokens, in seconds.
We also removed autoRefresh feature.

@ismailonly
Copy link

@JoaoPedroAS51 I believe that the 'expiresIn' feature is important.

  • The maxAge method, though simple and generic, forces us to rework on the already established standard JWT 'exp' claim.
  • Most JWTs are using the 'exp' claim, so we should provide this out of the box.
  • The 'exp' claim allows the server to set different expiries for different users based on its own security policies - which is an important use case.
  • The 'exp' claim promotes the principle of single source of truth where control is only with the server and the frontend developers don't need to worry about it.
  • Having a fixed maxAge and that too inside the config file is only fine when we don't want to touch it often

I humbly request you to reconsider and keep 'expiresIn' feature in v5 :)

@JoaoPedroAS51
Copy link
Collaborator

Hi @ismailonly! Thank you for your feedback! :)

I fully agree with you! And that's why we decode JWT tokens to get the exp ;)
In this case maxAge is a fallback.

It will be used by other tokens that can't be decoded.

The main reason we removed expiresIn is because we added decode feature. Do you think expiresIn still necessary?

@ismailonly
Copy link

@JoaoPedroAS51 I am sorry. My bad. I didn't read the docs properly which clearly states that the maxAge will be used when the decode fails.

Excellent. Thanks for the detailed responses. :)

@iteamaster89
Copy link

iteamaster89 commented May 14, 2020

Hi @JoaoPedroAS51, one more question concerns refresh:)
I tried to refresh when 401 occurs but with no success.
I used it in plugins when axios error occurs, lets say 401, smth like:
export default function ({$axios, app, store}) {
$axios.onError(error => {
if (error) {
app.$auth.refreshTokens();
}
})
}

Error occurs on server and it seems the only way to catch it.
May be there is an another way?

@JoaoPedroAS51
Copy link
Collaborator

JoaoPedroAS51 commented May 14, 2020

Hi @iteamaster89!

I think we can increment this plugin a little more :)
Don't forget to extend auth plugin, otherwise $auth will not be available.

NOT TESTED

export default function ({ $axios, $auth }) {
  $axios.onError(async ({ config, response: { status } }) => {
    // Only intercept requests with status 401
    // Don't intercept refresh token requests
    // And check if user is logged in and tokens are available
    if (status !== 401 || config.url === $auth.strategy.options.endpoints.refresh.url || !$auth.check()) {
      return Promise.reject(error)
    }

    // Refresh tokens before retrying current request
    await $auth.refreshTokens().catch((error) => {
      // Tokens couldn't be refreshed. Force reset.
      $auth.reset()
      throw error
    })

    // Set `isRetryRequest` flag to true
    config.__isRetryRequest = true

    // Update Authorization header
    config.headers[$auth.strategy.options.token.name] = $auth.token.get()

    // Retry original request
    return $axios.request(config)
  })
}

@JoaoPedroAS51
Copy link
Collaborator

@iteamaster89 I still recommending the approach of refresh tokens based on the token expiration instead of on error.

We adopted the practice of refresh tokens based on the token expiration. We intercept requests and check for token expiration. If the token has expired then we refresh. The original request will await for refresh request.

A 401 error means that you are unauthorized to access the target resource, but it doesn't mean that the token has expired. That's why we base the refresh on token expiration.

@iteamaster89
Copy link

Thanks man!

@iteamaster89
Copy link

iteamaster89 commented Jun 8, 2020

@JoaoPedroAS51 Hi, is it possible to dynamically change login method depending, for example, on cookie info ('mode=dev' or 'mode=prod')?
endpoints: {
login: { url: '/am/auth/v2/authorize', method: 'post' },
user: { url: '/am/main/v2/userinfo', method: 'get' },
}
May be there is a better way without using cookie...
Thanks in advance for your help!

@JoaoPedroAS51
Copy link
Collaborator

Hi @iteamaster89! If you want to change based on environment, I would suggest to use process.env vars in auth config, like (process.env.NODE_ENV !== 'production'). But if you want to change using cookies, then I would suggest to make a plugin that extends auth.

export default function ({ $auth }) {
  // do something here
  // you can access and change options using
  // $auth.strategies['local'].options.endpoints.login
}

@iteamaster89
Copy link

$auth.strategies['local'].options.endpoints.login

Thanks a lot! It's what I really needed.

@askerkhakh
Copy link

@iteamaster89 I still recommending the approach of refresh tokens based on the token expiration instead of on error.

We adopted the practice of refresh tokens based on the token expiration. We intercept requests and check for token expiration. If the token has expired then we refresh. The original request will await for refresh request.

Refresh strategy, based on token expiration time, is bit naive 😏. Even a small shift in time between the server and the client can lead to an unwanted 401 error. I think the refresh strategy should combine both the expiration time strategy and the interception of a 401 error.

A 401 error means that you are unauthorized to access the target resource, but it doesn't mean that the token has expired. That's why we base the refresh on token expiration.

This can be solved by checking an endpoint that is 100% available to the user, e.g. auth/me. If that endpoint returned 401 too, then we can say for sure that access token is expired.

@kevintechie
Copy link
Contributor

kevintechie commented Jul 3, 2020 via email

@askerkhakh
Copy link

askerkhakh commented Jul 15, 2020

Hi @iteamaster89!

I think we can increment this plugin a little more :)
Don't forget to extend auth plugin, otherwise $auth will not be available.

NOT TESTED

export default function ({ $axios, $auth }) {
  $axios.onError(async ({ config, response: { status } }) => {
    // Only intercept requests with status 401
    // Don't intercept refresh token requests
    // And check if user is logged in and tokens are available
    if (status !== 401 || config.url === $auth.strategy.options.endpoints.refresh.url || !$auth.check()) {
      return Promise.reject(error)
    }

    // Refresh tokens before retrying current request
    await $auth.refreshTokens().catch((error) => {
      // Tokens couldn't be refreshed. Force reset.
      $auth.reset()
      throw error
    })

    // Set `isRetryRequest` flag to true
    config.__isRetryRequest = true

    // Update Authorization header
    config.headers[$auth.strategy.options.token.name] = $auth.token.get()

    // Retry original request
    return $axios.request(config)
  })
}

Fixed version of this axios interceptor, hope it'll help someone

export default function ({$axios, $auth}) {
  $axios.onError(async (error) => {
    const config = error.config;
    // Don't intercept retry request
    // Only intercept requests with status 401
    // Don't intercept refresh token requests
    // And check if user is logged in and tokens are available
    if (
      config.__isRetryRequest ||
      error.response.status !== 401 ||
      config.url === $auth.strategy.options.endpoints.refresh.url ||
      !$auth.check()
    ) {
      return Promise.reject(error);
    }

    // Refresh tokens before retrying current request
    await $auth.refreshTokens().catch((error) => {
      // Tokens couldn't be refreshed. Force reset.
      $auth.reset();
      throw error;
    });

    // Set `isRetryRequest` flag to true
    config.__isRetryRequest = true;

    // Update Authorization header
    config.headers[$auth.strategy.options.token.name] = $auth.token.get();

    // Retry original request
    return $axios.request(config);
  });
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet