Adds custom proxy access dialog #382

alanshaw · 2018-02-13T16:02:33Z

window.confirm is not supported when called from a background script in Firefox
We need a mechanism to grant allow/deny to all permissions for a given origin

resolves #377

lidel · 2018-02-13T16:29:25Z

add-on/src/pages/proxy-access-dialog/page.js

+              <p class="sans-serif f6 lh-copy charcoal-muted">
+                <label>
+                  <input type="checkbox" checked=${state.remember} onclick=${onRememberToggle} class="mr1" />
+                  Apply to all permissions for ${origin}


Does Apply to all IPFS API permissions for sound better or worse?
(It is more clear to me, but I am non-native speaker, so.. 🙃 )

lidel · 2018-02-13T16:31:20Z

package.json

@@ -72,7 +72,7 @@
    "sinon": "4.1.2",
    "sinon-chrome": "2.2.1",
    "standard": "10.0.3",
-    "uglifyify": "^4.0.5",
+    "uglifyify": "4.0.5",


@alanshaw I think you forgot to update yarn.lock. If I run npm run yarn-build it modifies it:

--- a/yarn.lock +++ b/yarn.lock @@ -4374,7 +4374,7 @@ ipfs-block@^0.6.1, ipfs-block@~0.6.1: dependencies: cids "^0.5.2" -ipfs-css@^0.1.0: +[email protected]: version "0.1.0" resolved "https://registry.yarnpkg.com/ipfs-css/-/ipfs-css-0.1.0.tgz#0337e7e3aeddf391bbd1b7b7fb427f98b6ec5ca3" @@ -9315,7 +9315,7 @@ uglify-to-browserify@~1.0.0: version "1.0.2" resolved "https://registry.yarnpkg.com/uglify-to-browserify/-/uglify-to-browserify-1.0.2.tgz#6e0924d6bda6b5afe349e39a6d632850a0f882b7" -uglifyify@^4.0.5: +[email protected]: version "4.0.5" resolved "https://registry.yarnpkg.com/uglifyify/-/uglifyify-4.0.5.tgz#49c1fca9828c10a5a8e8d70f191a95f7ab475911" dependencies:

This is why Jenkins failed:

https://ci.ipfs.team/blue/organizations/jenkins/IPFS%20Shipyard%2Fipfs-companion/detail/PR-382/1/pipeline#step-14-log-29

alanshaw · 2018-02-14T10:02:58Z

When you check the checkbox I'm implementing something like this:

{
  origin: { '*': true }
}

Usually it would look like:

{
  origin: { 'files.add': true, 'object.new': false /* ... */ }
}

There's some nuances to * that I'd like to discuss before continuing:

What happens when we set access to *?
- Does it remove any existing permissions that have been granted? e.g. if I've already approved files.add and then I am asked about another permission for which I check the checkbox and click allow, does my permission object then look like { 'files.add': true, '*': true } or { '*': true }?
- If it removes all existing permissions, what happens if I try to set a specific grant? e.g. if I have { '*': true } and then set granted access to files.add to false should that fail or add the new grant? This is sort of hypothetical, because there's no way in the UI you can do this currently.
What happens when we get granted access to a permission when * exists?
- If a * grant exists is that the granted permission that the user gets or if there is a more specific permission, should that be returned instead? e.g. { '*': true, 'files.add': false } - what happens when I try to get granted access to files.add - do I get true or false?

Right now I'm leaning towards when you set * it removes all existing access grants and if you try to set a more specific access grant it fails - you can't set files.add: false if *: true (incidentally set files.add: true if *: true is a noop). Since this most closely matches the UX i.e. the fail will never happen as there is no UI for the user to add a more specific grant if * is already granted.

olizilla · 2018-02-14T10:12:21Z

Setting "allow all", I think it should remove more specific permissions, and just store { '*': true }.

If you hypothetically try to set a more specific permission, when { '*': true } is set, then I'd be fine with that being a no-op, in as much as, the the existing permissions already allow what you're asking for, so change nothing in the acl and say ok to the user.

If it should never be possible to set a more specific permission when a wildcard is already set, then an error would be appropriate. Right now that feels like an implicit rule just because of the way the ui is structured, but I don't think it's a rule we really care about.

alanshaw · 2018-02-15T10:14:34Z

@lidel @olizilla @victorbjelkholm ~~fonts from ipfs-css are not yet working but the main body of work here is done and I'd like to get some eyes on it so we can get it merged.~~ Could you please take a look?

~~I'll send a separate PR for getting the fonts working.~~

EDIT: fonts are in and working

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/child-src

lidel

Thank you @alanshaw, it is 👌 Awesome work! Works beautifully under Firefox 🚀

Is it ready to merge on your end? If so, see my 2 bikeshedding comments below.

ps. While testing this I noticed (unrelated) CSP warning and fixed it – just fetch my commits.

lidel · 2018-02-15T18:55:07Z

.gitignore

@@ -11,3 +11,5 @@ crowdin.yml
 add-on/dist
 coverage
 .nyc_output
+add-on/fonts/*
+add-on/*.css


Let's move 3rd-party stuff copied from ipfs-css and tachyons (fonts, css) into separate namespace, eg. add-on/ui-kit/ or something like that.

Note to self: our landing page (#324, #326) will re-use styles and fonts, eventually

lidel · 2018-02-15T19:02:17Z

add-on/src/lib/ipfs-proxy/pre-acl.js

-      const { allow } = await requestAccess(origin, permission)
-      access = await accessControl.setAccess(origin, permission, allow)
+      const { allow, remember } = await requestAccess(origin, permission)
+      access = await accessControl.setAccess(origin, remember ? '*' : permission, allow)


Took me a moment to get it.. remember does not seem to be a good description for what is happening here :)
Perhaps wildcard would be a more intuitive name than remember?

…p/ipfs-companion into feat/custom-proxy-access-dialog

alanshaw · 2018-02-19T14:34:19Z

@lidel good to merge?

adds custom proxy access dialog

26257f7

lidel reviewed Feb 13, 2018

View reviewed changes

lidel requested changes Feb 13, 2018

View reviewed changes

fixes yarn.lock

919cff7

lidel added this to the 2018-Q1 milestone Feb 13, 2018

lidel added the UX label Feb 13, 2018

alanshaw added 4 commits February 14, 2018 12:12

implements wildcard access grants

2d3a7cf

adds access control wildcard tests

90f9940

adds render test for proxy access dialog as sanity check

be38191

fixes remember checkbox

70278ab

alanshaw requested review from olizilla, lidel and victorb February 15, 2018 10:14

alanshaw changed the title ~~[WIP] Adds custom proxy access dialog~~ Adds custom proxy access dialog Feb 15, 2018

alanshaw and others added 4 commits February 15, 2018 11:14

removes browserify-css, adds scripts to copy css and fonts

6cd96aa

Merge branch 'master' into feat/custom-proxy-access-dialog

83c9ec4

Fix CSP deprecation warning

a08ad25

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/child-src

Preferences: fix UX of checkbox for window.ipfs

cf8197a

lidel reviewed Feb 15, 2018

View reviewed changes

lidel assigned alanshaw Feb 15, 2018

alanshaw added 3 commits February 19, 2018 14:10

moves ipfs.css, tachyons.css and fonts into ui-kit folder

f97edcb

Merge branch 'feat/custom-proxy-access-dialog' of github.com:tablefli…

ded5ca3

…p/ipfs-companion into feat/custom-proxy-access-dialog

reword variable remember to wildcard

efcedde

lidel approved these changes Feb 19, 2018

View reviewed changes

lidel merged commit 88eebce into ipfs:master Feb 19, 2018

alanshaw deleted the feat/custom-proxy-access-dialog branch February 19, 2018 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds custom proxy access dialog #382

Adds custom proxy access dialog #382

alanshaw commented Feb 13, 2018 •

edited

Loading

lidel Feb 13, 2018

lidel Feb 13, 2018

alanshaw commented Feb 14, 2018

olizilla commented Feb 14, 2018

alanshaw commented Feb 15, 2018 •

edited

Loading

lidel left a comment •

edited

Loading

lidel Feb 15, 2018

alanshaw Feb 19, 2018

lidel Feb 19, 2018

lidel Feb 15, 2018

alanshaw Feb 19, 2018

alanshaw commented Feb 19, 2018

Adds custom proxy access dialog #382

Adds custom proxy access dialog #382

Conversation

alanshaw commented Feb 13, 2018 • edited Loading

lidel Feb 13, 2018

Choose a reason for hiding this comment

lidel Feb 13, 2018

Choose a reason for hiding this comment

alanshaw commented Feb 14, 2018

olizilla commented Feb 14, 2018

alanshaw commented Feb 15, 2018 • edited Loading

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel Feb 15, 2018

Choose a reason for hiding this comment

alanshaw Feb 19, 2018

Choose a reason for hiding this comment

lidel Feb 19, 2018

Choose a reason for hiding this comment

lidel Feb 15, 2018

Choose a reason for hiding this comment

alanshaw Feb 19, 2018

Choose a reason for hiding this comment

alanshaw commented Feb 19, 2018

alanshaw commented Feb 13, 2018 •

edited

Loading

alanshaw commented Feb 15, 2018 •

edited

Loading

lidel left a comment •

edited

Loading