search

subdavis / Tusk

🐘 🔒 KeePass-compatible browser extension for filling passwords.
https://subdavis.com/Tusk
Other
482 stars 74 forks source link

🎉 Add advanced setting to allow all website access permission. #196

Closed zmilonas closed 6 years ago

zmilonas commented 6 years ago

Closes #168

Tested with Chrome 69 on macOS

Demo screen recording here: https://gfycat.com/ForthrightDefensiveIberianemeraldlizard

subdavis commented 6 years ago

Woohoo! Great demo gif, also!

Couple of quick questions about how you chose to do this.

  1. Permissions are usually requested in the background when the request comes from the pop-up page because the pop-up might close during the user interaction. There's no danger of the settings window closing, so you can skip that bit and run the permissions request directly from this page if you want.

  2. Could you talk about the choice to not allow users to revert this option? It looks like the user would have to modify permissions from the browser permissions manager rather than unclick this toggle. Would there be any issues with using the chrome.permissions.remove API to let users change their minds later?

BTW, I'm giving a talk about this project on Tuesday, and I have a slide to mention how cool it has been that contributors like you have come along to pick up features and bugs reports. Thanks!

zmilonas commented 6 years ago

@subdavis thanks for the feedback

ad 1. Good to know for the future :D ad 2. I must've been to careless when implementing and didn't know i can programatically remove the permission as well. I changed it up

Great to hear you like the contributions. Also, probably need a seperate issue for that but there is a need for some more or less centralized logging of errors, unless we want to just console.error to print to chrome extension log.

subdavis commented 6 years ago

@zmilonas looks like there are some conflicts here. Once those get sorted out, we can merge.

zmilonas commented 6 years ago

@subdavis conflicts resolved :)

subdavis commented 6 years ago

Opened #199 to tackle some of the anomalies in options page styles. Otherwise, LGTM.

subdavis commented 6 years ago

I don't know if I'm losing my mind, but this broke.

options.html:1 Unchecked runtime.lastError while running permissions.request: This function must be called during a user gesture
    at allOriginPermission (chrome-extension://fmhmiaejopepamlcjkncpgpdjichnecm/build/options.build.js:6602:24)
    at Ni.run (chrome-extension://fmhmiaejopepamlcjkncpgpdjichnecm/dll/dll.library.js:100:6484)
    at zt (chrome-extension://fmhmiaejopepamlcjkncpgpdjichnecm/dll/dll.library.js:95:74289)
    at Array.<anonymous> (chrome-extension://fmhmiaejopepamlcjkncpgpdjichnecm/dll/dll.library.js:95:68397)
    at MessagePort.at (chrome-extension://fmhmiaejopepamlcjkncpgpdjichnecm/dll/dll.library.js:95:68226)

I remember testing this PR and it working. I'm on Chromium 67.0.3396.99 (Official Build) Built on Ubuntu , running on Ubuntu 17.10 (64-bit)

This is now the same thing that Firefox does as mentioned in the README. Can you reproduce, @zmilonas? I'm running on commit 95ffcd96dd8ba1b11c1af307a59cedbdcb46fccc

zmilonas commented 6 years ago

@ubdavis Can't reproduce :/. What are the steps?

subdavis commented 6 years ago

Just try to toggle on this feature from advanced settings. What version of Chrome are you on, and what platform?