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 authentication plugin to embedded components #3298

Merged
merged 5 commits into from
Nov 17, 2022

Conversation

garrettjstevens
Copy link
Collaborator

This is a proposal that adds the ability to use internet accounts in the embedded React LGV and CGV. I've tested it with HTTP basic authentication.

The motivation for this is for use in jbrowse-jupyter (see this comment). There is a way to open local files (or at least files on the same machine as the jupyter/colab server) by taking advantage of the ability to invoke functions in the Python kernel from the notebook client. We can use that to read chunks of a file from disk instead of fetching them from a server. @teresam856 and I have been trying to think of a way to get this to work.

The proposed workflow would be to add tracks that have a UriLocation with a different protocol (file:// might work, but maybe it would have to be custom). Then for those tracks, jbrowse-jupyter would use a plugin to add an internet account with a custom getFetcher which would then invoke the Python kernel instead of fetching from a server.

An alternative to doing this with an internet account would be to use a custom filehandle and invoke the Python kernel there, but @rbuels and I couldn't figure out a way for a plugin to add and use a custom filehandle.

@garrettjstevens garrettjstevens self-assigned this Oct 28, 2022
@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Merging #3298 (783e603) into main (d54632d) will decrease coverage by 0.06%.
The diff coverage is 18.18%.

@@            Coverage Diff             @@
##             main    #3298      +/-   ##
==========================================
- Coverage   59.44%   59.38%   -0.07%     
==========================================
  Files         736      736              
  Lines       28997    29037      +40     
  Branches     7017     7029      +12     
==========================================
+ Hits        17238    17244       +6     
- Misses      11513    11547      +34     
  Partials      246      246              
Impacted Files Coverage Δ
products/jbrowse-desktop/src/rootModel.ts 30.49% <0.00%> (+0.27%) ⬆️
...owse-react-circular-genome-view/src/corePlugins.ts 100.00% <ø> (ø)
...r-genome-view/src/createModel/createConfigModel.ts 55.55% <ø> (ø)
...ircular-genome-view/src/createModel/createModel.ts 37.50% <0.00%> (-20.20%) ⬇️
...browse-react-linear-genome-view/src/corePlugins.ts 100.00% <ø> (ø)
...r-genome-view/src/createModel/createConfigModel.ts 55.55% <ø> (ø)
...-linear-genome-view/src/createModel/createModel.ts 38.46% <0.00%> (-21.54%) ⬇️
...-react-circular-genome-view/src/createViewState.ts 74.07% <20.00%> (-12.29%) ⬇️
...se-react-linear-genome-view/src/createViewState.ts 48.27% <20.00%> (-5.90%) ⬇️
products/jbrowse-web/src/rootModel.ts 55.03% <50.00%> (-0.33%) ⬇️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

configuration.type,
initializeInternetAccount(
internetAccountConfig: AnyConfigurationModel,
initialSnapshot = {},
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason for changing it to take a config instead of id?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the only place this is called, the calling code already has a handle the config. By passing it directly, we avoid having to call resolveIdentifier with the ID.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will note that things like onAction listeners cannot handle full configuration objects, and this is one reason why e.g. showTrack take a trackId even if they have a full conf object to supply generally. Internet account would probably rarely be attached to a onAction listener but it's just a data point for the use of ids in the api

@cmdcolin

This comment was marked as outdated.

@garrettjstevens
Copy link
Collaborator Author

garrettjstevens commented Nov 2, 2022

I'd tested this with HTTPBasicInternetAccount and it works fine, but I tried it with GoogleDriveOAuthInternetAccount and realized it won't work with that or with DropboxOAuthInternetAccount, either. This is because they rely on some code in JBrowse Web/Desktop to handle sending the login info from the popup window to the main page (e.g. see products/jbrowse-web/src/index.tsx). HTTPBasicInternetAccount works because it doesn't rely on that, it uses a dialog instead.

@cmdcolin
Copy link
Collaborator

added a small change to make it so that component_tests uses a built version of the auth plugin 6d6531b

@cmdcolin
Copy link
Collaborator

should be good to go? i wasn't sure what token to paste in the storybook example that @garrettjstevens added but seems ok :)

@cmdcolin cmdcolin merged commit b903476 into main Nov 17, 2022
@cmdcolin cmdcolin deleted the internetAccount_embedded branch November 17, 2022 19:31
@cmdcolin cmdcolin changed the title Add ability to use internet accounts in embedded Add authentication plugin to embedded components Nov 17, 2022
@garrettjstevens
Copy link
Collaborator Author

Yep, this is good to go.

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

2 participants