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

Always proxy oncokb request #2864

Merged
merged 1 commit into from
Nov 15, 2019
Merged

Always proxy oncokb request #2864

merged 1 commit into from
Nov 15, 2019

Conversation

zhx828
Copy link
Member

@zhx828 zhx828 commented Nov 11, 2019

The coming up new OncoKB website is no longer allowing cross domain requests. All requests need to be proxied and attached the auth header

The new OncoKB website is no longer allowing cross domain requests. All requests need to be proxied and attach the auth header
if (typeof url === 'string') {
return buildCBioPortalAPIUrl(`proxy/${trimProtocol(url)}`);
} else {
return undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does calling code handle undefined? I guess typescript would complain if it didn't, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, we inject these url to the client directly bypassing the constructer. It will have problem if url is undefined. But it seems like we will never run into this scenario since we have the serverConfigDefaults defined. @alisman

Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

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

@zhx828 This looks fine to me, but does it still make sense to allow configuration of the OncoKB url on the frontend? Since now it's entirely through the proxy, maybe it shouldn't even be configurable

@zhx828
Copy link
Member Author

zhx828 commented Nov 15, 2019

@inodb I agree. This is a fix to make sure any instances that do not wish to upgrade to 3.2(which oncokb auth will be included) can run smoothly. This will eliminate a cross-domain issue if the oncokb instance does not allow that.
In the 3.2, I will remove oncokb_public_url from the frontend.

@zhx828
Copy link
Member Author

zhx828 commented Nov 15, 2019

I took a look places where oncokb is used. They all work properly.

@zhx828 zhx828 merged commit ef7e5eb into cBioPortal:master Nov 15, 2019
@inodb inodb added the external api This PR is related to handling an external API label Nov 15, 2019
@zhx828 zhx828 deleted the update-oncokb branch January 6, 2020 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external api This PR is related to handling an external API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants