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

Setting OAuth token globally is broken? #1344

Closed
victorskl opened this issue Jun 9, 2021 · 10 comments
Closed

Setting OAuth token globally is broken? #1344

victorskl opened this issue Jun 9, 2021 · 10 comments

Comments

@victorskl
Copy link

I'm setting OAuth token globally like this as described in Wiki entry.

igv.oauth.setToken(accessToken, "*htsget*");

It seems oauth.js is refactored into igv-utils. Version 2.7.4 is the last release to able to set OAuth token globally this way.

Will it be possible to reinstate this feat? Or, must set token for each track?

Thanks

victorskl added a commit to umccr/data-portal-client that referenced this issue Jun 9, 2021
* Bumped igv.js 1.8.5
* Downside is need to include `igv-utils` explicitly
  from github. No npm package avail for `igv-utils`.
  See igvteam/igv.js#1344
@victorskl
Copy link
Author

Just to further note, fallback to use oauthToken for track config didn't work for htsgetReader as track oauthToken this.config at this line does not propagate to subsequence htsget data fetch calls. Hence, only first htsget ticket call contain bearer auth token.

@jrobinso
Copy link
Contributor

jrobinso commented Jun 9, 2021

@victorskl Both of the issues you mention here would be bugs, I will look into it.

@jrobinso
Copy link
Contributor

jrobinso commented Jun 9, 2021

I have exposed the "oauth" object so. igv.oauth.setToken(accessToken, "htsget") should now work. This will be kept for backward compatibility for some time, however I also added a new function that does not expose that object that should be used going forward

igv.setOauthToken(accessToken, host)

@jrobinso
Copy link
Contributor

jrobinso commented Jun 9, 2021

@victorskl I think I've fixed both of these issues now (global and track setting). However, I have no way to test htsget protocol, if you know of a public endpoint I can use for testing please note it here.

@victorskl
Copy link
Author

Thanks Jim for the fixes, will try again once it get release and report.

re: htsget public endpoint

Yep, please ditto to @brainstorm pointer in #1187

Plus my notes as follows:

I understand you don't prefer running one; I will see what we can do about it. Meantime, if you won't mind running local through Docker, then there are some pointer in those ☝️ links.

Thanks again! Cheers

@jrobinso
Copy link
Contributor

@victorskl I've been trying to add support for variants, but not getting very far. You can see my progress or lack thereof at #1187 (scroll to bottom) . It feels like I'm an early adopter here. However, in doing that I noticed a bug in the "fix" I pushed from per track oAuth. I don't think it will work, the headers from the htsget "ticket" will probably override the oAuth header. I will fix that and leave this open until I think its fixed. However the global setting should still work.

@victorskl
Copy link
Author

[...] per track oAuth. I don't think it will work, the headers from the htsget "ticket" will probably override the oAuth header. I will fix that and leave this open until I think its fixed.

Yes Jim, you are absolutely right about that. I also realise that after I made my remark. It looks like it is server responsibility to provide how to access the (data block) URLs i.e. pass along OAuth token for client to include as auth Bearer header... TBH, this is a bit awkward in Htsget spec for typical SPA / app xhr request flow though. But yah, let us stick to the spec. Happy to roll back per track OAuth behaviour to previous way. If need be, I will work it out at backend server side.

However the global setting should still work.

Yep; this is still handy feat.

Thanks

@jrobinso
Copy link
Contributor

@victorskl So if I understand you correctly, if the server returns an "authorization bearer" header with the urls this should override any token set initially in the track config? In that case the code as published in the last release should work as is. I will try to set up some simulated mode to test this. In any event respond here if there are further problems.

@victorskl
Copy link
Author

So if I understand you correctly, if the server returns an "authorization bearer" header with the urls this should override any token set initially in the track config?

Yes, Jim.

Client have to use all headers hinted by Server for fetching data; that includes authorization bearer token. Though, I reckon this flow is rarely the case. In that sense, at track config, Client has to use (override if any) this auth bearer header.

victorskl added a commit to umccr/data-portal-client that referenced this issue Jun 15, 2021
* Use setOauthToken
  See igvteam/igv.js#1344
* Bump other deps
@victorskl
Copy link
Author

It is working! Happy to close this. Thanks again Jim.

Cheers!

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

No branches or pull requests

2 participants