-
Notifications
You must be signed in to change notification settings - Fork 325
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
window.ipfs #333
window.ipfs #333
Changes from 16 commits
5e9e2a1
2d13c37
51c0afe
c1d4c98
f21fcac
e061b6f
0eeb425
c0eb828
ab8300f
d49cf04
521e909
87af28f
de6de37
01ae150
9b29486
9b0009d
af59463
3886340
cbeed1a
d02c787
9aa0f8e
4aa6408
c5a7ee1
9237bfb
5dd1116
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,3 +9,5 @@ yarn-error.log | |
crowdin.yml | ||
.*~ | ||
add-on/dist | ||
coverage | ||
.nyc_output |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
'use strict' | ||
|
||
const browser = require('webextension-polyfill') | ||
const injectScript = require('./inject-script') | ||
|
||
function init () { | ||
const port = browser.runtime.connect({ name: 'ipfs-proxy' }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If other extensions know this name, is it possible they can connect to it? That would be bad, as they could bypass the permissions you've setup, if I understand correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would! I'll check it out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...although thinking about it, the access control is all done in the extension background process, so it doesn't matter who's connecting and asking for stuff, it still has to be authorised by the user. I'll double check but I think it might be ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok so, the background process is listening to
It would need to listen to Aside from that, even if connections from other extensions were allowed, the authorization is done in the background process, so the user would still need to have approved the function to be called before the proxy server let it through to the IPFS instance. |
||
|
||
// Forward on messages from background to the page and vice versa | ||
port.onMessage.addListener((data) => window.postMessage(data, '*')) | ||
|
||
window.addEventListener('message', (msg) => { | ||
if (msg.data && msg.data.sender === 'postmsg-rpc/client') { | ||
port.postMessage(msg.data) | ||
} | ||
}) | ||
|
||
injectScript(browser.extension.getURL('dist/contentScripts/ipfs-proxy/page.js')) | ||
} | ||
|
||
// Only run this once for this window! | ||
// URL can change (history API) which causes this script to be executed again, | ||
// but it only needs to be setup once per window... | ||
if (!window.__ipfsProxyContentInitialized) { | ||
init() | ||
window.__ipfsProxyContentInitialized = true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
'use strict' | ||
|
||
function injectScript (src, target, opts) { | ||
opts = opts || {} | ||
const doc = opts.document || document | ||
|
||
const scriptTag = doc.createElement('script') | ||
scriptTag.src = src | ||
scriptTag.onload = function () { | ||
this.parentNode.removeChild(this) | ||
} | ||
|
||
target = doc.head || doc.documentElement | ||
target.appendChild(scriptTag) | ||
} | ||
|
||
module.exports = injectScript |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
'use strict' | ||
|
||
const Ipfs = require('ipfs') | ||
const { createProxyClient } = require('ipfs-postmsg-proxy') | ||
const _Buffer = Buffer | ||
|
||
window.Buffer = window.Buffer || _Buffer | ||
window.Ipfs = window.Ipfs || Ipfs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should expose both Is there a good use case for exposing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe you want an IPFS node configured with particular swarm peers so using the one provided by the extension is not possible? ...and you don't want to download the IPFS lib because you're on a poor connection? Maybe...for experiments? Are they good enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say yes, it would make developer's life easier. 👍 @diasdavid, would appreciate your thoughts on this from |
||
window.ipfs = window.ipfs || createProxyClient() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ const { createIpfsUrlProtocolHandler } = require('./ipfs-protocol') | |
const createNotifier = require('./notifier') | ||
const createCopier = require('./copier') | ||
const { createContextMenus, findUrlForContext } = require('./context-menus') | ||
const createIpfsProxy = require('./ipfs-proxy') | ||
|
||
// init happens on addon load in background/background.js | ||
module.exports = async function init () { | ||
|
@@ -26,8 +27,10 @@ module.exports = async function init () { | |
var copier | ||
var contextMenus | ||
var apiStatusUpdateInterval | ||
var ipfsProxy | ||
const offlinePeerCount = -1 | ||
const idleInSecs = 5 * 60 | ||
const browserActionPortName = 'browser-action-port' | ||
|
||
try { | ||
const options = await browser.storage.local.get(optionDefaults) | ||
|
@@ -43,9 +46,14 @@ module.exports = async function init () { | |
onCopyAddressAtPublicGw: () => copier.copyAddressAtPublicGw() | ||
}) | ||
modifyRequest = createRequestModifier(getState, dnsLink, ipfsPathValidator) | ||
ipfsProxy = createIpfsProxy(() => ipfs, getState) | ||
registerListeners() | ||
await setApiStatusUpdateInterval(options.ipfsApiPollMs) | ||
await storeMissingOptions(options, optionDefaults, browser.storage.local) | ||
await storeMissingOptions( | ||
await browser.storage.local.get(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain this change? The code looks fine, just curious. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! Previously On first run, const options = await browser.storage.local.get(optionDefaults) Regarding the parameter to
...so, it finds So Similarly for subsequent runs, options will be assigned any missing defaults so there's never anything to store! Instead, we get a fresh copy of the current options, without defaults applied. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch! I think I've made this bug while porting from legacy SDK to WebExtension APIs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...probably should be in a separate PR - apologies |
||
optionDefaults, | ||
browser.storage.local | ||
) | ||
} catch (error) { | ||
console.error('Unable to initialize addon due to error', error) | ||
if (notify) notify('notify_addonIssueTitle', 'notify_addonIssueMsg') | ||
|
@@ -119,7 +127,6 @@ module.exports = async function init () { | |
// e.g. signalling between browser action popup and background page that works | ||
// in everywhere, even in private contexts (https://github.com/ipfs/ipfs-companion/issues/243) | ||
|
||
const browserActionPortName = 'browser-action-port' | ||
var browserActionPort | ||
|
||
function onRuntimeConnect (port) { | ||
|
@@ -518,6 +525,8 @@ module.exports = async function init () { | |
state.dnslink = change.newValue | ||
} else if (key === 'preloadAtPublicGateway') { | ||
state.preloadAtPublicGateway = change.newValue | ||
} else if (key === 'ipfsProxy') { | ||
state.ipfsProxy = change.newValue | ||
} | ||
} | ||
} | ||
|
@@ -554,6 +563,8 @@ module.exports = async function init () { | |
notify = null | ||
copier = null | ||
contextMenus = null | ||
ipfsProxy.destroy() | ||
ipfsProxy = null | ||
await destroyIpfsClient() | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
'use strict' | ||
|
||
const EventEmitter = require('events') | ||
const PQueue = require('p-queue') | ||
|
||
class AccessControl extends EventEmitter { | ||
constructor (storage, key = 'ipfsProxyAcl') { | ||
super() | ||
this._storage = storage | ||
this._key = key | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if we store ACL keys with a prefix? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, just to confirm our keys would look like Currently things look sort of like this: {
ipfsProxyAcl: {
'http://google.co.uk': {
'files.add': true
},
'https://ipfs.io': {
'files.add': false,
'object.new': false
}
}
} ...and we'd be altering it to be something like: {
'ipfsProxyAcl.http://google.co.uk': {
'files.add': true
},
'ipfsProxyAcl.https://ipfs.io': {
'files.add': false,
'object.new': false
}
} I like this and I think it makes things simpler, the only issue I can think of is getting the entire ACL for the manage permissions page might be tricky - we'd have to read everything in storage into memory and filter just the keys we need. I think that's a better trade of though since atm we're reading the entire ACL into memory for every There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like a plan 👌 |
||
this._onStorageChange = this._onStorageChange.bind(this) | ||
storage.onChanged.addListener(this._onStorageChange) | ||
this._writeQ = new PQueue({ concurrency: 1 }) | ||
} | ||
|
||
async _onStorageChange (changes) { | ||
const isAclChange = Object.keys(changes).some((key) => key === this._key) | ||
if (!isAclChange) return | ||
const acl = await this.getAcl() | ||
this.emit('change', acl) | ||
} | ||
|
||
async getAccess (origin, permission) { | ||
const acl = await this.getAcl() | ||
if (acl[origin] == null || acl[origin][permission] == null) return null | ||
return { origin, permission, allow: acl[origin][permission] } | ||
} | ||
|
||
async setAccess (origin, permission, allow) { | ||
return this._writeQ.add(async () => { | ||
const access = { origin, permission, allow } | ||
const acl = await this.getAcl() | ||
|
||
acl[origin] = acl[origin] || {} | ||
acl[origin][permission] = allow | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard against the forbidden origins like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's less pretty, but might be safer to structure the acl with data that comes from the outside as values rather than keys... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a |
||
|
||
await this._setAcl(acl) | ||
return access | ||
}) | ||
} | ||
|
||
// { origin: { permission: allow } } | ||
async getAcl () { | ||
const acl = (await this._storage.local.get(this._key))[this._key] | ||
return acl ? JSON.parse(acl) : {} | ||
} | ||
|
||
_setAcl (acl) { | ||
return this._storage.local.set({ [this._key]: JSON.stringify(acl) }) | ||
} | ||
|
||
// Revoke access to the given permission | ||
// if permission is null, revoke all access | ||
async revokeAccess (origin, permission = null) { | ||
return this._writeQ.add(async () => { | ||
const acl = await this.getAcl() | ||
|
||
if (permission) { | ||
delete acl[origin][permission] | ||
} else { | ||
acl[origin] = {} | ||
} | ||
|
||
return this._setAcl(acl) | ||
}) | ||
} | ||
|
||
destroy () { | ||
this._storage.onChanged.removeListener(this._onStorageChange) | ||
} | ||
} | ||
|
||
module.exports = AccessControl |
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.
Please append key names at the end of descriptions, eg:
This is so that crowdin users can see it as additional hint about message's context.
(Crowdin UI hides key names for some reason 🤔 )
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.
Sure 👍