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

feat: add support for custom headers to send-request #741

Merged
merged 6 commits into from Jun 18, 2018

Conversation

Projects
None yet
7 participants
@OR13
Copy link
Contributor

commented Apr 15, 2018

This pull request adds a header object to the config, which can be used to provide default headers on all requests made by this library.

This is useful when IPFS is behind http proxies that require custom headers, such as Authorization.

@daviddias
Copy link
Member

left a comment

@OR13 thanks for adding this in. Mind adding a test and documenting this option?

@OR13

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

I have added a test, and the build is passing in node 8, but not node 6.

https://travis-ci.org/ipfs/js-ipfs-api/jobs/370753539#L4269

@OR13

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@diasdavid is a passing node 6 build a requirement for this PR?

likewise, this test only covers the node case. is that acceptable?

@prettymuchbryce

This comment has been minimized.

Copy link

commented Apr 25, 2018

I think we may also need to add documentation, although I'm not completely sure where that lives. My best guess would be some mention in README.md.

@OR13

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

@prettymuchbryce I added a short blurb under the CORS setup.

I misinterpreted the documentation request initially, sorry for the delay.

If you would like me to add additional detail about why custom headers are useful, I can do that.

I'm trying to keep this as simple as possible, as its not an option everyone will need.

@prettymuchbryce

This comment has been minimized.

Copy link

commented Apr 26, 2018

Your short blurb in the README makes sense to me. I think it's perfectly fine for now.

In terms of the tests, it looks like all of them are failing so I'm guessing it's not related to your change. It looks like Jenkins may be out of disk space. I'm not sure who's responsible for taking a look into that.

@OR13

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2018

Jenkins is failing, but travis is passing:

https://travis-ci.org/ipfs/js-ipfs-api/builds/371638643

error sha3@1.2.1: The engine "node" is incompatible with this module. Expected version ">= 8.11.1".
error An unexpected error occurred: "Found incompatible module".
@prettymuchbryce

This comment has been minimized.

Copy link

commented May 1, 2018

@diasdavid Mind pointing us in the right direction with the testing issues? Anything we can do to help?

@victorb

This comment has been minimized.

Copy link
Member

commented May 1, 2018

I've rerun the tests on jenkins as it was still using a old nodejs version we switched yesterday. Change was made here: ipfs/jenkins-libs@54eca5d

I rerun with 8.11.1 activated, testrun is here: https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fjs-ipfs-api/detail/PR-741/6/pipeline

port: 5001,
protocol: 'http',
headers: {
authorization: 'Bearer ' + TOKEN

This comment has been minimized.

Copy link
@lgierth

lgierth May 1, 2018

Member

I'm not familiar with the http server lib we're using here -- does it transparently convert header keys to camel case?

This comment has been minimized.

Copy link
@OR13

OR13 May 3, 2018

Author Contributor

@lgierth I've often wondered about casing of http headers, especially across languages:

https://stackoverflow.com/questions/5258977/are-http-headers-case-sensitive

Here are come examples of headers being set prior to this change.

https://github.com/ipfs/js-ipfs-api/blob/master/src/utils/send-request.js#L113

Its worth noting that user agent header will be overwritten by this code even if its provided in the config.

The headers object is passed directly to the request library:

https://github.com/ipfs/js-ipfs-api/blob/master/src/utils/send-request.js#L162

https://github.com/ipfs/js-ipfs-api/blob/master/src/utils/request.js

Here is an interesting blog post on header casing and some challenges it can cause:

https://medium.com/walmartlabs/hacking-node-core-http-module-f2a10ea3028e

This comment has been minimized.

Copy link
@OR13

OR13 May 3, 2018

Author Contributor

long story short, i don't think header casing should be altered by code in this library.

This comment has been minimized.

Copy link
@mikeal

mikeal May 31, 2018

Member

This is actually a pretty hard and annoying problem, particularly in Node.js.

The basic rule of thumb is that you need to preserve casing on user inputs. This is very problematic if you need to check and manipulate headers elsewhere in code.

I wrote a library to deal with this in request, so it's pretty widely used. https://github.com/request/caseless, basically just wraps the headers object and provides an API for you to manipulate it in a caseless way while still maintaining the case of the user inputs.

@OR13

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

The test that is failing, passes for me locally, and passes on travis...

  it('.ping with default n', (done) => {
    ipfs.ping(otherId, (err, res) => {
      expect(err).to.not.exist()
      expect(res).to.be.an('array')
      expect(res).to.have.lengthOf(3)
      res.forEach(packet => {
        expect(packet).to.have.keys('Success', 'Time', 'Text')
        expect(packet.Time).to.be.a('number')
      })
      const resultMsg = res.find(packet => packet.Text.includes('Average latency'))
      expect(resultMsg).to.exist()
      done()
    })
  })

Example expected output:

[ { Success: true,
    Time: 0,
    Text: 'PING QmQF7qFBAzMdXr8NyMR7uQV7WYuafmpC1xjCsdbmGZ6PDP.' },
  { Success: true, Time: 225158, Text: '' },
  { Success: true, Time: 0, Text: 'Average latency: 0.23ms' } ]

Not sure what I can do to fix this build issue, any advice is welcome.

I see that its WIP: #762

@alanshaw

This comment has been minimized.

Copy link
Member

commented May 15, 2018

@OR13 If all your tests have passed I think that's something we need to solve, hopefully once we have, a rebase and retest should get this passing. I'll come back to you asap.

@daviddias daviddias changed the title add support for custom headers to send-request feat: add support for custom headers to send-request May 29, 2018

@prettymuchbryce

This comment has been minimized.

Copy link

commented Jun 2, 2018

@alanshaw Any updates here? Would be great to be able to authenticate with our nodes.

@daviddias daviddias added the ready label Jun 4, 2018

@@ -106,7 +106,7 @@ function requestAPI (config, options, callback) {
delete options.qs.followSymlinks

const method = 'POST'
const headers = {}
const headers = config.headers || {}

This comment has been minimized.

Copy link
@alanshaw

alanshaw Jun 4, 2018

Member

We need an Object.assign({}, config.headers) to avoid mutating the headers object passed as config, otherwise subsequent requests will receive the same headers as previous.

jheinnic added a commit to jheinnic/js-ipfs-api that referenced this pull request Jun 14, 2018

Merge pull request #1 from ipfs/master
Preparing a local merged copy of PR ipfs#741
fix: clone config.headers to avoid prev headers in next req
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>

@ghost ghost assigned alanshaw Jun 18, 2018

@ghost ghost added in progress and removed ready labels Jun 18, 2018

@alanshaw

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

CI is currently having issues, running the tests locally passes. Thank you @OR13 🚀

@alanshaw alanshaw merged commit 7fb2e07 into ipfs:master Jun 18, 2018

2 of 9 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
ci/jenkins/commitlint Linting failed
Details
ci/jenkins/macos/10.4.1/test Tests failed
Details
ci/jenkins/windows/10.4.1/test Tests failed
Details
ci/jenkins/linux/8.11.3/test Tests in progress
Details
ci/jenkins/macos/8.11.3/test Tests in progress
Details
ci/jenkins/windows/8.11.3/test Tests in progress
Details
ci/jenkins/codelint Linting passed
Details
ci/jenkins/linux/10.4.1/test Tests passed
Details

@ghost ghost removed the in progress label Jun 18, 2018

danieldaf added a commit to danieldaf/js-ipfs-api that referenced this pull request Jul 21, 2018

feat: add support for custom headers to send-request (ipfs#741)
* add support for custom headers to send-request

* add custom headers test

* add custom header documentation

* fix: clone config.headers to avoid prev headers in next req

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.