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

Add OAuth token refresh #3466

Closed

Conversation

andrzejgrzelak
Copy link
Contributor

This PR adds workaround fix of #3194.
It executes getPreAuthorizationInformation() directly from InternetAccountModel getFetcher(), first checking if it's used in web worker context to prevent the web worker thread from trying to open new windows.

Also fixes the issue of multiple token exchanges by executing storeToken() immediately after token has been exchanged in validateToken().

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Jan 23, 2023
@cmdcolin
Copy link
Collaborator

cmdcolin commented Jan 24, 2023

great work on this @andrzejgrzelak

I really need to create a little oauth sandbox environment so i can test stuff like this better

the alternative to this could be just changing assembly loading back into an RPC, it was originally changed from RPC->main thread to help bundle size but webpack4->webpack5 conversion made the RPC bundles much smaller, so could consider switching back

Copy link
Collaborator

@garrettjstevens garrettjstevens left a comment

Choose a reason for hiding this comment

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

I'm working on a way to reliably reproduce #3194 locally, but did have one request for now about moving storeToken, see inline comment.

@andrzejgrzelak
Copy link
Contributor Author

I'm working on a way to reliably reproduce #3194 locally, but did have one request for now about moving storeToken, see inline comment.

My issue was that if there were X multiple tracks with same oAuth InternetAccount and token required a refresh it was refreshed X times. This storeToken was mitigating that, but it might be I'm missing something.

@andrzejgrzelak
Copy link
Contributor Author

Sorry for such late action, I thought that you can fix my error, didn't realize I have to do it myself.

@cmdcolin
Copy link
Collaborator

cmdcolin commented Apr 5, 2023

thanks for updating @andrzejgrzelak

@garrettjstevens was working on adding more oauth testing here in the meantime to help validate changes like this so that was what was taking some of our time too #3572

@cmdcolin cmdcolin added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Apr 6, 2023
@cmdcolin cmdcolin changed the title Add oauth token refresh Add OAuth token refresh Apr 6, 2023
@cmdcolin
Copy link
Collaborator

may close for now, this PR will be superseded by #3572 (comment) which found a couple additional things that are needed

@cmdcolin cmdcolin closed this May 18, 2023
@cmdcolin
Copy link
Collaborator

will update this thread when it is merged though :)

@cmdcolin
Copy link
Collaborator

cmdcolin commented Jun 7, 2023

@andrzejgrzelak work on token refresh has been merged to the main branch, can now get a new copy via jbrowse create --nightly newinst to check out

it may not be totally fixed yet, i think it may be launching prompts to login even after it properly refreshes a token, but it is hopefully a bit better now and we have a good system for simulating a variety of oauth conditions locally with a test server

@cmdcolin
Copy link
Collaborator

cmdcolin commented Jun 9, 2023

v2.6.1 released with this change added!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants