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 correct access control headers to the default api config #1979

Merged
merged 3 commits into from
Dec 9, 2015

Conversation

dignifiedquire
Copy link
Member

Nedded to fix ipfs-inactive/js-ipfs-http-client#76 and for ipfs-inactive/js-ipfs-http-client#128 for details why this is needed see: ipfs-inactive/js-ipfs-http-client#76 (comment)


Probably lots wrong with this, so please excuse any obvious mistakes

@daviddias
Copy link
Member

LGTM, thank you for catching this one @dignifiedquire :)

For more context: https://botbot.me/freenode/ipfs/2015-11-19/?msg=54525489&page=9

@whyrusleeping
Copy link
Member

@dignifiedquire you'll need to fix the type your using in the config init:

HTTPHeaders: map[string][]string{

@daviddias
Copy link
Member

@whyrusleeping I was looking at that and wondering if it would make more sense to have the config coming from the JSON blob, instead of .go generating the config (this way could be one of those global things that other implementations use)

@whyrusleeping
Copy link
Member

@diasdavid i'm not sure what youre saying. The JSON blob gets loaded into this struct so its easier to work with in go. (types are nice, mkay)

@daviddias
Copy link
Member

@whyrusleeping If I'm reading it right, when ipfs init is done, the config json file is created and then loaded from there. But what if we shipped the default config in a json file with the code?

@dignifiedquire
Copy link
Member Author

Will fix the type in the morning..

@whyrusleeping
Copy link
Member

@diasdavid, some of the config is generated dynamically to make changing certain values easier.

"Access-Control-Allow-Headers": "X-Stream-Output, X-Chunked-Output",
"Access-Control-Expose-Headers": "X-Stream-Output, X-Chunked-Output",
},
},
Copy link
Member

Choose a reason for hiding this comment

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

add them also to Gateway, because gateway also has an API. (we need to rename these... not sure on good names)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jbenet sorry not sure where exactly, do you mean on l. 83?

@dignifiedquire dignifiedquire force-pushed the fix/access-controll-headers branch 2 times, most recently from 54fa7dc to dc4b819 Compare November 20, 2015 08:38
@dignifiedquire
Copy link
Member Author

@whyrusleeping I fixed the tests, anything else that I should do?

@jbenet
Copy link
Member

jbenet commented Nov 20, 2015

I meant add the same headers to the "Gateway" config key as well.
On Fri, Nov 20, 2015 at 11:17 Friedel Ziegelmayer notifications@github.com
wrote:

@whyrusleeping https://github.com/whyrusleeping I fixed the tests,
anything else that I should do?


Reply to this email directly or view it on GitHub
#1979 (comment).

@dignifiedquire
Copy link
Member Author

@jbenet done (or so I think)

@whyrusleeping
Copy link
Member

this LGTM, though it seems kinda hacky, is this a config option we would ever want to change? if not, maybe it should go into the gateway code, and be hardcoded in?

@dignifiedquire
Copy link
Member Author

@whyrusleeping tbh you know this better than I, these headers are directly dependent on the headers you are using in the api, so as long as you don't change those you don't have to change these here.

@dignifiedquire
Copy link
Member Author

@jbenet @whyrusleeping we really need this in 0.3.10 this is causing issues for a lot of people trying to use js-ipfs-api. Do you need anything else from my side?

License: MIT
Signed-off-by: Friedel Ziegelmayer <dignifiedquire@gmail.com>
License: MIT
Signed-off-by: Friedel Ziegelmayer <dignifiedquire@gmail.com>
License: MIT
Signed-off-by: Friedel Ziegelmayer <dignifiedquire@gmail.com>
@dignifiedquire dignifiedquire force-pushed the fix/access-controll-headers branch from 7286afe to b036b23 Compare November 25, 2015 19:11
@jbenet
Copy link
Member

jbenet commented Nov 30, 2015

well, i think if this is needed it shouldn't be a config value-- +1 to hardcoding this into where we set the defaults.

@jbenet jbenet merged commit b036b23 into ipfs:master Dec 9, 2015
@jbenet jbenet removed the backlog label Dec 9, 2015
@dignifiedquire dignifiedquire deleted the fix/access-controll-headers branch December 9, 2015 11:15
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

Successfully merging this pull request may close these issues.

findprovs returns object or string
4 participants