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

Fix for expired token set #9

Closed
wants to merge 1 commit into from

Conversation

nicogarcia
Copy link
Contributor

No description provided.

@joshcanhelp
Copy link
Contributor

@nicogarcia - I know this has been open for a while but can you give me a bit of context on this? I'm not sure if we should require a login if tokens are expired (access tokens are not tied to the user session).

@nicogarcia
Copy link
Contributor Author

nicogarcia commented Sep 25, 2019

Sure! Basically the tokens can be expired and using them would produce an error, so that's the bug I've identified (and suffered 😅), the decision on what to do is follows what I understood of the lib at the time of creating this PR, I don't recall perfectly but those tokens should be updated to be able to use them against apis, and requiredLogin was the variable used to start the login flow that probably doesn't have any prompts if the Auth0 session is still valid.

This is how I patched this in my code to re-request the tokens if expired: https://github.com/auth0/growth-portal/blob/master/server/index.js#L105

FYI I'm ooo, but I'll try to check back, lmk in slack if I stop responding 🙂

@joshcanhelp
Copy link
Contributor

joshcanhelp commented Sep 27, 2019

Thanks for the context @nicogarcia ...

the tokens can be expired

Do you know what expiration time that it uses? I'm guessing the one returned with the access token, since that's right in the response.

and using them would produce an error

That's to be expected. The application should expect those to expire and obtain new ones with a refresh token when that happens.

those tokens should be updated to be able to use them against apis

The problem here, though, is that access tokens can be used without a user around, or even without a valid session. Triggering a login should happen if there is no session with the application and the user is trying to take an action that requires one, not if the application's access to an API has expired. If the application needs long-term access to that API, then you should be requesting offline_access and keeping the refresh token around.

Vittorio has a video explanation of all this here. Working with refresh tokens should happen something like this (we're working on reducing the steps for this a bit):

// ...
app.get('/expenses', requiresAuth(), async (req, res, next) => {
  try {
    let tokenSet = req.openid.tokens;
    if(tokenSet.expired()) {
      tokenSet = await req.openid.client.refresh(tokenSet);
      tokenSet.refresh_token = req.openid.tokens.refresh_token;
      req.openid.tokens = tokenSet;
    }
    // ...
  }
  // ...
});

@joshcanhelp
Copy link
Contributor

I'm going to close this one for now. This will cause the session length to essentially be set by the access token expiration time (as I understand it). So if you had a short-lived access token of, say, an hour, but an application session of 7 days, you're now being forced to log in when the access token expires. As I mentioned above, the way to get a new access token is either using a refresh token or the API-calling portion of the application checking the access token and forcing a redirect if no refresh token is present.

@nicogarcia
Copy link
Contributor Author

Sorry, I was oof. All your points make sense, refreshing the tokens without prompting login is the right way, thanks!

@nicogarcia nicogarcia deleted the fix_expired_tokens branch October 25, 2019 13:43
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.

None yet

2 participants