-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
License: MIT Signed-off-by: Henrique Dias <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to test this against real basic auth setup in a spare moment today/tomorrow, for now quick feedback below.
Thanks @lidel! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
So this PR works as expected 👍
Remaining work in this PR:
-
src/js-ipfs-api/index.test.js
needs tests for API specified as URL (with and without basic auth)
There are bugs in WebUI that need to be addressed upstream
(it does not support object, just string with multiaddr, details below)
HTTP Proxy for testing this
Ok, there are many moving pieces, including Origin validation at go-ipfs, so I've wrote a custom http proxy to make it easier to test:
#!/usr/bin/env node
const http = require('http')
const httpProxy = require('http-proxy')
const basicAuth = require('basic-auth')
// ============================================================
// CONFIG
// ============================================================
const port = 5403 // port on which we want to enable basic auth
const target = 'http://127.0.0.1:5001' // where /api/v0/ lives
// basic auth:
const user = 'user'
const password = 'pass'
// ============================================================
const proxy = httpProxy.createProxyServer()
proxy.on('proxyReq', (proxyReq, req, res, options) => {
// to make this poc easier, swap Origin so internal check of go-ipfs does get triggered (403 Forbidden on Origin mismatch)
proxyReq.setHeader('Origin', target);
proxyReq.setHeader('Referer', target);
proxyReq.setHeader('Host', new URL(target).host);
})
http.createServer((req, res) => {
console.log(`${req.method}\t\t${req.url}`)
res.oldWriteHead = res.writeHead
res.writeHead = function(statusCode, headers) {
// hardcoded liberal CORS for easier testing
res.setHeader('Access-Control-Allow-Origin', '*')
// usual suspects + 'authorization' header
res.setHeader('Access-Control-Allow-Headers', 'X-Stream-Output, X-Chunked-Output, X-Content-Length, authorization')
res.oldWriteHead(statusCode)
}
const auth = basicAuth(req)
const preflight = req.method === 'OPTIONS' // need preflight working
if (!preflight && !(auth && auth.name === user && auth.pass === password)) {
res.writeHead(401, {
'WWW-Authenticate': 'Basic realm="IPFS API"'
})
res.end('Access denied')
} else {
proxy.web(req, res, { target })
}
}).listen(port)
TL;DR: You run it via node ./proxy.js
, then confirm it works via curl http://user:[email protected]:5403/api/v0/id
It should save us a TON of time with setting CORS headers.
Testing WebUI with this PR
connect-deps makes it easy to test webui with this PR: (in your local webui repo) connect-deps link ../ipfs-redux-bundle/
, then connect-deps connect -w
Test result
Clicking on "Submit" in WebUI did nothing:
So instead I set ipfsApi
hint for WebUI via console:
localStorage.setItem('ipfsApi','http://user:[email protected]:5403')
Reloaded WebUI and ipfs-redux-bundle
from this PR picked URL up correctly:
I was able to open http://127.0.0.1:3000/#/files
and browse files using HTTP API over Basic Auth.
Bugs in WebUI
Found some bugs in WebUI tho (writing them down below, but need to be addressed upstream):
-
I was unable to set URL via
http://localhost:3000/#/welcome
, Submit button does nothing, I had to set it via console like described above. -
When set via console, the Connection screen atfixed in latest version of this PRhttp://localhost:3000/#/welcome
displays broken value for URL (looks like invalid string serialization):
-
Status page at
http://127.0.0.1:3000/#/files
crash because it expects multiaddr:
License: MIT Signed-off-by: Henrique Dias <[email protected]>
License: MIT Signed-off-by: Henrique Dias <[email protected]>
@lidel I moved the checks and conversion of the api address to the Thanks for that proxy!!!!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just address small ask below and ok to ship it.
FYSA this fixed WebUI Bug (2) (URL set via console now looks ok), remaining bugs need to be addressed upstream, in against WebUI which switches to new ipfs-redux-bundle
if (addr === null || addr === undefined || typeof addr === 'undefined') { | ||
return false | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sneaked in during refactoring, can be removed, try/catch below is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is on isMultiaddr
and not on isUrl
. Our multiaddr
does not throw with undefined nor null. I'll leave this check, but the one on isUrl
was removed.
License: MIT Signed-off-by: Henrique Dias <[email protected]>
License: MIT Signed-off-by: Henrique Dias <[email protected]>
Reintroducing this in ipfs/ipfs-webui#1613 |
This should fix ipfs/ipfs-webui#836 by allowing to pass custom URLs.
I was not able to try it due to some CORS configuration with Basic Auth I couldn't manage to get working right now. Will try later. @lidel could you take a look at this to see if it looks OK or even try it if you've got time?
License: MIT
Signed-off-by: Henrique Dias [email protected]