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: Refresh access_token if expired #24

Merged
merged 9 commits into from Nov 5, 2020
Merged

Fix: Refresh access_token if expired #24

merged 9 commits into from Nov 5, 2020

Conversation

@fcjr
Copy link
Member

@fcjr fcjr commented Nov 5, 2020

The browser removes the access_token if expired however it's likely we have a refresh_token available to get a new one, so rather than failing and prompting the user to sign in again, we can just try to renew here first.

fcjr added 4 commits Nov 5, 2020
@fcjr fcjr requested review from sammacbeth and chrmod Nov 5, 2020
Copy link
Contributor

@sammacbeth sammacbeth left a comment

I think a simpler solution would be just to call call AccessToken.refresh() if we're missing an access token, then if this is successful generateTokens() will automatically get called again.

src/background.js Outdated Show resolved Hide resolved
src/token-pool.js Outdated Show resolved Hide resolved
@fcjr
Copy link
Member Author

@fcjr fcjr commented Nov 5, 2020

I think a simpler solution would be just to call call AccessToken.refresh() if we're missing an access token, then if this is successful generateTokens() will automatically get called again.

The problem there is that even though generateTokens is called again async; in the request that needs the tokens (here:

const token = await tokenPool.getToken();
) getToken will still return undefined causing the search to fail.

@sammacbeth
Copy link
Contributor

@sammacbeth sammacbeth commented Nov 5, 2020

I think a simpler solution would be just to call call AccessToken.refresh() if we're missing an access token, then if this is successful generateTokens() will automatically get called again.

The problem there is that even though generateTokens is called again async; in the request that needs the tokens (here:

const token = await tokenPool.getToken();

) getToken will still return undefined causing the search to fail.

I see. I'm not sure holding the outgoing search request before we get tokens is the right approach. Ideally we should always have tokens ready for sync usage if the user is logged in. This could then be easily fixed by just fetching tokens on startup, then we can keep the token pool stocked up over time in the background.

I think that's a simpler solution architecture-wise than making the AccessToken.get() function do a refresh if needed, and hold off resolution until the cookie update event fires (which it may never do).

fcjr added 3 commits Nov 5, 2020
@fcjr
Copy link
Member Author

@fcjr fcjr commented Nov 5, 2020

I think a simpler solution would be just to call call AccessToken.refresh() if we're missing an access token, then if this is successful generateTokens() will automatically get called again.

The problem there is that even though generateTokens is called again async; in the request that needs the tokens (here:

const token = await tokenPool.getToken();

) getToken will still return undefined causing the search to fail.

I see. I'm not sure holding the outgoing search request before we get tokens is the right approach. Ideally we should always have tokens ready for sync usage if the user is logged in. This could then be easily fixed by just fetching tokens on startup, then we can keep the token pool stocked up over time in the background.

I think that's a simpler solution architecture-wise than making the AccessToken.get() function do a refresh if needed, and hold off resolution until the cookie update event fires (which it may never do).

What about just trying to refresh the token in these two scenarios:

  1. it is not found on startup
  2. on a removed event

this should basically mean the pool should always be populated as it will always cause generateTokens to be called on startup if possible, and it will keep the access_token refreshed (which in turn keeps the token pool filled)

@chrmod
chrmod approved these changes Nov 5, 2020
@chrmod chrmod merged commit a1a2872 into master Nov 5, 2020
@chrmod chrmod deleted the fix/token_refresh branch Nov 5, 2020
chrmod added a commit to ghostery/user-agent-desktop that referenced this pull request Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants